Model rounding mode control for floating-point intrinsics #226

eopXD · 2023-04-28T03:09:41Z

This PR resembles #222 and aims to model rounding mode (frm) control for the floating-point intrinsics.

What is different to what we have done to the fixed-point intrinsics is that we will be keeping the existing floating-point intrinsics since not all users care about or need to alter the rounding mode.

// Keeping the existing interfaces
vfloat32m1_t __riscv_vfadd_vv_f32m1 (vfloat32m1_t op1, vfloat32m1_t op2, size_t vl);
vfloat32m1_t __riscv_vfadd_vv_f32m1_m (vbool32_t mask, vfloat32m1_t op1, vfloat32m1_t op2, size_t vl);
// Adding the variants with `_rm` suffix the controls the rounding mode
+ vfloat32m1_t __riscv_vfadd_vv_f32m1_rm (vfloat32m1_t op1, vfloat32m1_t op2, unsigned int frm, size_t vl);
+ vfloat32m1_t __riscv_vfadd_vv_f32m1_rm_m (vbool32_t mask, vfloat32m1_t op1, vfloat32m1_t op2, unsigned int frm, size_t vl);

The LLVM implementation is supported the following:

auto-generated/llvm-overloaded-tests/vfncvt_rod.c

auto-generated/api-testing/vfrsqrt7.c

auto-generated/api-testing/vwcvt.c

auto-generated/llvm-api-tests/vfwcvt.c

auto-generated/llvm-overloaded-tests/vfneg.c

auto-generated/api-testing/vfwcvt.c

Hi, Richi. We (RVV) is going to add a rounding mode operand into floating-point instructions which have 11 operands. Since we are going have intrinsic that is adding rounding mode argument: riscv-non-isa/rvv-intrinsic-doc#226 This is the patch that is adding rounding mode operand in RISC-V port: https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618573.html You can see there are 11 operands in these patterns. Is it Ok for trunk ? Thanks gcc/ChangeLog: * optabs.cc (maybe_gen_insn): Add case to generate instruction that has 11 operands.

We (RVV) is going to add a rounding mode operand into floating-point instructions which have 11 operands. Since we are going have intrinsic that is adding rounding mode argument: riscv-non-isa/rvv-intrinsic-doc#226 This is the patch that is adding rounding mode operand in RISC-V port: https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618573.html You can see there are 11 operands in these patterns. gcc/ChangeLog: * optabs.cc (maybe_gen_insn): Add case to generate instruction that has 11 operands. Signed-off-by: Juzhe-Zhong <juzhe.zhong@rivai.ai>

zhongjuzhe · 2023-05-17T15:30:56Z

Hi, I have these following questions:

Will you add enum like FRM_RNE similiar with fixed-point ?
Is it right we also need to model rounding mode in argument "unsigned int frm“ instead of the current API you posted in this PR ？

zhongjuzhe · 2023-05-29T06:07:55Z

vfcvt.f.xu.v vd, vs2, vm/vfcvt.f.x.v vd, vs2, vm
vfwcvt.f.xu.v vd, vs2, vm/vfwcvt.f.x.v vd, vs2, vm
vfwcvt.f.f.v vd, vs2, vm
vfncvt.f.xu.w vd, vs2, vm/vfncvt.f.x.w vd, vs2, vm
vfncvt.f.f.w vd, vs2, vm

Do these instructions need to have intrinsic with modeling rounding mode?

eopXD · 2023-05-31T17:15:33Z

vfcvt.f.xu.v vd, vs2, vm/vfcvt.f.x.v vd, vs2, vm vfwcvt.f.xu.v vd, vs2, vm/vfwcvt.f.x.v vd, vs2, vm vfwcvt.f.f.v vd, vs2, vm vfncvt.f.xu.w vd, vs2, vm/vfncvt.f.x.w vd, vs2, vm vfncvt.f.f.w vd, vs2, vm

Do these instructions need to have intrinsic with modeling rounding mode?

Yes I think so. I have just updated the PR.

auto-generated/api-testing/vfadd.c

auto-generated/api-testing/vfcvt.c

auto-generated/api-testing/vfrec7.c

auto-generated/api-testing/vfredmax.c

auto-generated/api-testing/vfredosum.c

auto-generated/api-testing/vfwcvt.c

auto-generated/api-testing/vfwredosum.c

auto-generated/api-testing/vwcvtu.c

According to doc: https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/222/files riscv-non-isa/rvv-intrinsic-doc#226 Add __RISCV_ prefix to VXRM and FRM enum. gcc/ChangeLog: * config/riscv/riscv-vector-builtins.cc (DEF_RVV_VXRM_ENUM): Add __RISCV_ prefix. (DEF_RVV_FRM_ENUM): Ditto. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/frm-1.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-1.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-10.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-11.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-12.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-6.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-7.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-8.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-9.c: Ditto.

eopXD · 2023-06-15T11:59:31Z

Change: Fix suffix for policy overloaded versions.

zixuan-wu · 2023-06-28T01:58:55Z

I am wondering if we need a dynamic mode enum which representing using current vxrm value? @eopXD

topperc · 2023-06-28T05:08:52Z

auto-generated/api-testing/vfadd.c

@@ -244,3 +244,243 @@ vfloat64m8_t test_vfadd_vf_f64m8_m(vbool8_t mask, vfloat64m8_t op1, float64_t op
  return __riscv_vfadd_vf_f64m8_m(mask, op1, op2, vl);
 }

+vfloat16mf4_t test_vfadd_vv_f16mf4_rm(vfloat16mf4_t op1, vfloat16mf4_t op2, size_t vl) {
+  return __riscv_vfadd_vv_f16mf4_rm(op1, op2, __RISCV_FRM_RNE, vl);


Are the __RISCV_FRM_RNE and other values documented? The web view makes it hard to find if it is.

Changes within this repo generally will break into at least two part: 1) generator/doc update and 2) re-run generator.

So personally I will switch to the commit tab and found those doc change for easier review :)

e.g.
f1ad325

dzaima · 2023-07-02T12:27:10Z

Is there a reason the _rm suffix is after the policy suffix? Currently, as far as I know, the policy suffix is always the at the end, which allows automatically transforming (i.e. via C macro concatenation or otherwise) any instruction to one with specific policy via appending to it, but this would introduce the need to sometimes put it in the middle instead.

In my opinion, it would be better to have _rm before the policy, i.e. __riscv_vfadd_vv_f32m1_rm_m, such that __riscv_vfadd_vv_f32m1_rm by itself is a regular intrinsic that can be masked by appending to its name.

eopXD · 2023-07-03T07:33:22Z

Is there a reason the _rm suffix is after the policy suffix? Currently, as far as I know, the policy suffix is always the at the end, which allows automatically transforming (i.e. via C macro concatenation or otherwise) any instruction to one with specific policy via appending to it, but this would introduce the need to sometimes put it in the middle instead.

In my opinion, it would be better to have _rm before the policy, i.e. __riscv_vfadd_vv_f32m1_rm_m, such that __riscv_vfadd_vv_f32m1_rm by itself is a regular intrinsic that can be masked by appending to its name.

It was more natural to me when I created the PR that the _rm is appending "after" the policy suffix because we are keeping the existing ones with semantic that does not alternate the rounding mode and use what is already within frm.

However I think you made a valid point, I will update the PR.

On the other hand, I see your good work https://dzaima.github.io/intrinsics-viewer/ that will be helpful for the RVV intrinsics users. I sent an email that is linked with your Github commits <dzaimagit [at] gmail.com>, please reply to me if possible, thank you.

Depends on D152879. Specification PR: riscv-non-isa/rvv-intrinsic-doc#226 This patch adds variant of `vfadd` that models the rounding mode control. The added variant has suffix `_rm` appended to differentiate from the existing ones that does not alternate `frm` and uses whatever is inside. The value `7` is used to indicate no rounding mode change. Reusing the semantic from the rounding mode encoding for scalar floating-point instructions. Additional data member `HasFRMRoundModeOp` is added so we can append `_rm` suffix for the fadd variants that models rounding mode control. Additional data member `IsRVVFixedPoint` is added so we can define pseudo instructions with rounding mode operand and distinguish the instructions between fixed-point and floating-point. Reviewed By: craig.topper, kito-cheng Differential Revision: https://reviews.llvm.org/D152996

According to doc: https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/222/files riscv-non-isa/rvv-intrinsic-doc#226 Add __RISCV_ prefix to VXRM and FRM enum. gcc/ChangeLog: * config/riscv/riscv-vector-builtins.cc (DEF_RVV_VXRM_ENUM): Add __RISCV_ prefix. (DEF_RVV_FRM_ENUM): Ditto. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/frm-1.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-1.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-10.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-11.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-12.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-6.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-7.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-8.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-9.c: Ditto.

According to the doc as below, we need to support the rounding mode of the RVV floating-point, both the static and dynamice frm. riscv-non-isa/rvv-intrinsic-doc#226 For tracking and development friendly, We will take some steps to support all rounding modes for the RVV floating-point rounding modes. 1. Allow rounding mode control by one intrinsic (aka this patch), vfadd. 2. Support static rounding mode control by mode switch, like fixed-point. 3. Support dynamice round mode control by mode switch. 4. Support the rest floating-point instructions for frm. Please *NOTE* this patch only allow the rounding mode control for the vfadd intrinsic API, and the related frm will be coverred by step 2. Signed-off-by: Pan Li <pan2.li@intel.com> Co-Authored by: Juzhe-Zhong <juzhe.zhong@rivai.ai> gcc/ChangeLog: * config/riscv/riscv-protos.h (enum floating_point_rounding_mode): Add macro for static frm min and max. * config/riscv/riscv-vector-builtins-bases.cc (class binop_frm): New class for floating-point with frm. (BASE): Add vfadd for frm. * config/riscv/riscv-vector-builtins-bases.h: Likewise. * config/riscv/riscv-vector-builtins-functions.def (vfadd_frm): Likewise. * config/riscv/riscv-vector-builtins-shapes.cc (struct alu_frm_def): New struct for alu with frm. (SHAPE): Add alu with frm. * config/riscv/riscv-vector-builtins-shapes.h: Likewise. * config/riscv/riscv-vector-builtins.cc (function_checker::report_out_of_range_and_not): New function for report out of range and not val. (function_checker::require_immediate_range_or): New function for checking in range or one val. * config/riscv/riscv-vector-builtins.h: Add function decl. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/float-point-frm-error.c: New test. * gcc.target/riscv/rvv/base/float-point-frm.c: New test.

According to doc: https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/222/files riscv-non-isa/rvv-intrinsic-doc#226 Add __RISCV_ prefix to VXRM and FRM enum. gcc/ChangeLog: * config/riscv/riscv-vector-builtins.cc (DEF_RVV_VXRM_ENUM): Add __RISCV_ prefix. (DEF_RVV_FRM_ENUM): Ditto. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/frm-1.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-1.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-10.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-11.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-12.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-6.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-7.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-8.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-9.c: Ditto.

According to the doc as below, we need to support the rounding mode of the RVV floating-point, both the static and dynamice frm. riscv-non-isa/rvv-intrinsic-doc#226 For tracking and development friendly, We will take some steps to support all rounding modes for the RVV floating-point rounding modes. 1. Allow rounding mode control by one intrinsic (aka this patch), vfadd. 2. Support static rounding mode control by mode switch, like fixed-point. 3. Support dynamice round mode control by mode switch. 4. Support the rest floating-point instructions for frm. Please *NOTE* this patch only allow the rounding mode control for the vfadd intrinsic API, and the related frm will be coverred by step 2. Signed-off-by: Pan Li <pan2.li@intel.com> Co-Authored by: Juzhe-Zhong <juzhe.zhong@rivai.ai> gcc/ChangeLog: * config/riscv/riscv-protos.h (enum floating_point_rounding_mode): Add macro for static frm min and max. * config/riscv/riscv-vector-builtins-bases.cc (class binop_frm): New class for floating-point with frm. (BASE): Add vfadd for frm. * config/riscv/riscv-vector-builtins-bases.h: Likewise. * config/riscv/riscv-vector-builtins-functions.def (vfadd_frm): Likewise. * config/riscv/riscv-vector-builtins-shapes.cc (struct alu_frm_def): New struct for alu with frm. (SHAPE): Add alu with frm. * config/riscv/riscv-vector-builtins-shapes.h: Likewise. * config/riscv/riscv-vector-builtins.cc (function_checker::report_out_of_range_and_not): New function for report out of range and not val. (function_checker::require_immediate_range_or): New function for checking in range or one val. * config/riscv/riscv-vector-builtins.h: Add function decl. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/float-point-frm-error.c: New test. * gcc.target/riscv/rvv/base/float-point-frm.c: New test.

According to below RVV intrinsic doc, the RVV floating-point intrinsic name with rounding mode should be: _rm_m instead of: _m_rm riscv-non-isa/rvv-intrinsic-doc#226 This patch fix this naming sequence issue and adjust the test cases. Signed-off-by: Pan Li <pan2.li@intel.com> gcc/ChangeLog: * config/riscv/riscv-vector-builtins-shapes.cc (struct alu_frm_def): Move rm suffix before mask. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/float-point-frm-insert-1.c: Adjust test cases. * gcc.target/riscv/rvv/base/float-point-frm.c: Ditto.