Model rounding mode control for fixed-point intrinsics #222

eopXD · 2023-04-25T17:24:05Z

This PR is based on previous RFC post, this replaces the existing fixed-point intrinsics and adds new fixed-point intrinsics with prefix implying the control to the fixed-point rounding mode register vxrm.

Edit: Through discussion below, the current PR is not going to static encode the rounding mode into the function name and is using an parameter that is restricted for constant inputs only.

- vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, unsigned int vxrm, size_t vl);

The LLVM implementation are the following:

Note that vsadd(u) and vssub(u) don't have the additional operand because computation of the instructions don't consider the rounding mode.

topperc · 2023-04-25T17:28:33Z

Why not make the rounding mode a function argument?

eopXD · 2023-04-25T17:34:56Z

Why not make the rounding mode a function argument?

Yes this is also a choice. If as a function argument, with constraint of the value to be a constant, I think it is only a difference in style between the current proposal, which is statically encoding them in the function name.

I think my intuition comes from that the policy behaviors are also statically encoded in the function name.

topperc · 2023-04-25T17:41:11Z

Why not make the rounding mode a function argument?

Yes this is also a choice. If as a function argument, with constraint of the value to be a constant, I think it is only a difference in style between the current proposal, which is statically encoding them in the function name.

I think my intuition comes from that the policy behaviors are also statically encoded in the function name.

The policy affects the number of function arguments in some cases so I think it had to be that way. That's not the case for rounding mode.

Making it an argument makes it easier to use as a macro argument or template parameter. Not sure if that's useful.

It also reduces the number of symbols, and I think makes the translation from C to LLVM IR intrinsics easier? We will want it as an argument in IR intrinsics.

nick-knight · 2023-04-25T18:38:49Z

I agree, the the main reason we encoded "vtype" (output EEW, output EMUL, tail policy, mask policy) in the intrinsics names was because C doesn't support overloading (although some dialects do), and vtype affects the number and types of function arguments.

I think you are correct that vxrm doesn't affect the number and types of arguments, so we have more flexibility in this case. I prefer @eopXD's proposal for a superficial reason, that it feels to me to be more similar to the existing API. Plus, I'm not exactly sure how to constrain a C function argument to be a compile-time constant.

Regarding using the C preprocessor to facilitate generic programming, we're already well accustomed to accommodating the existing API with the usual tricks. Supporting vxrm could be achieved similarly. We could also define C++ wrappers that expose vxrm via a template non-type parameter.

In any event, switching the fixed-point rounding mode tends to be an involved algorithmic decision, considered on an instruction-by-instruction basis. So I don't anticipate much demand for facilitating generic-vxrm programming. You typically do the math, determine the appropriate mode, then set it & forget it.

eopXD · 2023-04-26T06:22:24Z

Although I personally don't consider the the number of intrinsics increased as a critical decision point between "static encode in function name" and "adding an extra parameter", I think I should still list them out here in detail so we can have full discussion.

As we want floating-point and fixed-point intrinsics have aligned naming scheme, and assuming 25% of instruction in RVV is floating-point instructions and 20% of instructions in RVV is fixed-point instructions, lets consider the increased amount of intrinsics. Note that we intend to keep the original floating-point intrinsics and remove the existing fixed-point intrinsics. That gives us:

Static encode in function name results in 1/4 * 6 + 1/5 * 4 + (1 - 1/4 - 1/5) = 2.85, which is x2.85 of the current amount.
Adding an extra parameter results in 1/4 * 2 + 1/5 + 1 - 1/4 - 1/5, which is x1.25 of the current amount.

kito-cheng · 2023-04-28T09:46:25Z

I would support using parameter rather than static encode here, 2.85x is too huge number...

eopXD · 2023-05-05T09:45:47Z

Change of interface as Kito is not a big fan of the growth of intrinsics.

Changing the addition from:

- vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rnu (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rne (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rdn (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rod (vint32m1_t op1, vint32m1_t op2, size_t vl);

to

- vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vxrm, size_t vl);

PR description is updated.

topperc · 2023-05-10T22:48:51Z

Change of interface as Kito is not a big fan of the growth of intrinsics.

Changing the addition from:

- vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rnu (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rne (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rdn (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1_rod (vint32m1_t op1, vint32m1_t op2, size_t vl);

to

- vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
+ vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vxrm, size_t vl);

PR description is updated.

You can probably use unsigned int for the vxrm argument. It's required to be a constant so there shouldn't be any extend instructions generated for it being less than xlen.

zhongjuzhe · 2023-05-15T06:11:05Z

It seems that you missed vssrl/vssra instructions ?

I saw RVV ISA SPEC:

These instructions shift the input value right, and round off the shifted out bits according to vxrm. The scaling right shifts have both zero-extending (vssrl) and sign-extending (vssra) forms. The data to be shifted is in the vector register group specified by vs2 and the shift amount value can come from a vector register group vs1, a scalar integer register rs1, or a zero-extended 5-bit immediate. Only the low lg2(SEW) bits of the shift-amount value are used to control the shift amount.

for vssrl/vssra

eopXD · 2023-05-15T07:52:59Z

It seems that you missed vssrl/vssra instructions ?

I saw RVV ISA SPEC:

These instructions shift the input value right, and round off the shifted out bits according to vxrm. The scaling right shifts have both zero-extending (vssrl) and sign-extending (vssra) forms. The data to be shifted is in the vector register group specified by vs2 and the shift amount value can come from a vector register group vs1, a scalar integer register rs1, or a zero-extended 5-bit immediate. Only the low lg2(SEW) bits of the shift-amount value are used to control the shift amount.

for vssrl/vssra

Nice catch. Fixed it in the latest commit.

Since we are going to have fixed-point intrinsics that are modeling rounding mode riscv-non-isa/rvv-intrinsic-doc#222 We should have operand to specify rounding mode in fixed-point instructions. We don't support these modeling rounding mode intrinsics yet but we will definetely support them later. This is the preparing patch for new coming intrinsics. gcc/ChangeLog: * config/riscv/riscv-protos.h (enum vxrm_field_enum): New enum. * config/riscv/riscv-vector-builtins.cc (function_expander::use_exact_insn): Add default rounding mode operand. * config/riscv/riscv.cc (riscv_hard_regno_nregs): Add VXRM_REGNUM. (riscv_hard_regno_mode_ok): Ditto. (riscv_conditional_register_usage): Ditto. * config/riscv/riscv.h (DWARF_FRAME_REGNUM): Ditto. (VXRM_REG_P): Ditto. (RISCV_DWARF_VXRM): Ditto. * config/riscv/riscv.md: Ditto. * config/riscv/vector.md: Ditto Signed-off-by: Juzhe-Zhong <juzhe.zhong@rivai.ai>

zhongjuzhe · 2023-05-16T14:07:24Z

Are you going to add VXRM_RNE.... enum to user?

Hi, since fixed-point with modeling rounding mode intrinsics are coming: riscv-non-isa/rvv-intrinsic-doc#222 I am adding vxrm rounding mode enum to user first before the API intrinsic. This patch is simple && obvious. Ok for trunk ? gcc/ChangeLog: * config/riscv/riscv-vector-builtins.cc (register_vxrm): New function. (DEF_RVV_VXRM_ENUM): New macro. (handle_pragma_vector): Add vxrm enum register. * config/riscv/riscv-vector-builtins.def (DEF_RVV_VXRM_ENUM): New macro. (RNU): Ditto. (RNE): Ditto. (RDN): Ditto. (ROD): Ditto. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/vxrm-1.c: New test.

zhongjuzhe · 2023-05-17T10:11:15Z

Hi, could you update the enum in this PR today ?
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618827.html
GCC patch is waiting for this PR update to get merged.

Thanks

Hi, since fixed-point with modeling rounding mode intrinsics are coming: riscv-non-isa/rvv-intrinsic-doc#222 I am adding vxrm rounding mode enum to user first before the API intrinsic. This patch is simple && obvious. Ok for trunk ? gcc/ChangeLog: * config/riscv/riscv-vector-builtins.cc (register_vxrm): New function. (DEF_RVV_VXRM_ENUM): New macro. (handle_pragma_vector): Add vxrm enum register. * config/riscv/riscv-vector-builtins.def (DEF_RVV_VXRM_ENUM): New macro. (RNU): Ditto. (RNE): Ditto. (RDN): Ditto. (ROD): Ditto. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/vxrm-1.c: New test.

According to new comming fixed-point API: riscv-non-isa/rvv-intrinsic-doc#222 Introduce vxrm argument: - vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl); + vint32m1_t __riscv_vsadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vxrm, size_t vl); This patch doesn't insert vxrm csrw configuration instruction yet. Will support automatically insert csrw vxrm instruction in the next patch. This patch does this following: 1. Only extend the vxrm argument. 2. Check vxrm argument is invalid immediate and report error message if it is invalid. gcc/ChangeLog: * config/riscv/riscv-vector-builtins-bases.cc: Introduce rounding mode. * config/riscv/riscv-vector-builtins-shapes.cc (struct alu_def): Ditto. (struct narrow_alu_def): Ditto. * config/riscv/riscv-vector-builtins.cc (function_builder::apply_predication): Ditto. (function_expander::use_exact_insn): Ditto. * config/riscv/riscv-vector-builtins.h (function_checker::arg_num): New function. (function_base::has_rounding_mode_operand_p): New function. gcc/testsuite/ChangeLog: * g++.target/riscv/rvv/base/bug-11.C: Adapt testcase. * g++.target/riscv/rvv/base/bug-12.C: Ditto. * g++.target/riscv/rvv/base/bug-14.C: Ditto. * g++.target/riscv/rvv/base/bug-15.C: Ditto. * g++.target/riscv/rvv/base/bug-16.C: Ditto. * g++.target/riscv/rvv/base/bug-17.C: Ditto. * g++.target/riscv/rvv/base/bug-18.C: Ditto. * g++.target/riscv/rvv/base/bug-19.C: Ditto. * g++.target/riscv/rvv/base/bug-20.C: Ditto. * g++.target/riscv/rvv/base/bug-21.C: Ditto. * g++.target/riscv/rvv/base/bug-22.C: Ditto. * g++.target/riscv/rvv/base/bug-23.C: Ditto. * g++.target/riscv/rvv/base/bug-3.C: Ditto. * g++.target/riscv/rvv/base/bug-5.C: Ditto. * g++.target/riscv/rvv/base/bug-6.C: Ditto. * g++.target/riscv/rvv/base/bug-8.C: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-100.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-101.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-102.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-103.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-104.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-105.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-106.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-107.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-108.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-109.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-110.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-111.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-112.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-113.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-114.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-115.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-116.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-117.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-118.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-119.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-122.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-97.c: Ditto. * gcc.target/riscv/rvv/base/binop_vx_constraint-98.c: Ditto. * gcc.target/riscv/rvv/base/merge_constraint-1.c: Ditto. * gcc.target/riscv/rvv/base/narrow_constraint-6.c: Ditto. * gcc.target/riscv/rvv/base/narrow_constraint-7.c: Ditto. * gcc.target/riscv/rvv/base/narrow_constraint-8.c: Ditto. * gcc.target/riscv/rvv/base/narrow_constraint-9.c: Ditto. * gcc.target/riscv/rvv/base/vxrm-2.c: New test. * gcc.target/riscv/rvv/base/vxrm-3.c: New test. * gcc.target/riscv/rvv/base/vxrm-4.c: New test. * gcc.target/riscv/rvv/base/vxrm-5.c: New test.

… for fixed-point instructions Hi, this patch support the new coming fixed-point intrinsics: riscv-non-isa/rvv-intrinsic-doc#222 Insert fixed-point rounding mode configuration by mode switching target hook. Mode switching target hook is implemented applying LCM (Lazy code Motion). So the performance && correctness can be well trusted. Here is the example: void f (void * in, void *out, int32_t x, int n, int m) { for (int i = 0; i < n; i++) { vint32m1_t v = __riscv_vle32_v_i32m1 (in + i, 4); vint32m1_t v2 = __riscv_vle32_v_i32m1_tu (v, in + 100 + i, 4); vint32m1_t v3 = __riscv_vaadd_vx_i32m1 (v2, 0, VXRM_RDN, 4); v3 = __riscv_vaadd_vx_i32m1 (v3, 3, VXRM_RDN, 4); __riscv_vse32_v_i32m1 (out + 100 + i, v3, 4); } for (int i = 0; i < n; i++) { vint32m1_t v = __riscv_vle32_v_i32m1 (in + i + 1000, 4); vint32m1_t v2 = __riscv_vle32_v_i32m1_tu (v, in + 100 + i + 1000, 4); vint32m1_t v3 = __riscv_vaadd_vx_i32m1 (v2, 0, VXRM_RDN, 4); v3 = __riscv_vaadd_vx_i32m1 (v3, 3, VXRM_RDN, 4); __riscv_vse32_v_i32m1 (out + 100 + i + 1000, v3, 4); } } ASM: ... csrwi vxrm,2 vsetivli zero,4,e32,m1,tu,ma ... Loop 1 ... Loop 2 mode switching can global recognize both Loop 1 and Loop 2 are using RDN rounding mode and hoist such single "csrwi vxrm,2" to dominate both Loop 1 and Loop 2. Besides, I have add correctness check sanity tests in this patch too. Ok for trunk ? gcc/ChangeLog: * config/riscv/riscv-opts.h (enum riscv_entity): New enum. * config/riscv/riscv.cc (riscv_emit_mode_set): New function. (riscv_mode_needed): Ditto. (riscv_mode_after): Ditto. (riscv_mode_entry): Ditto. (riscv_mode_exit): Ditto. (riscv_mode_priority): Ditto. (TARGET_MODE_EMIT): New target hook. (TARGET_MODE_NEEDED): Ditto. (TARGET_MODE_AFTER): Ditto. (TARGET_MODE_ENTRY): Ditto. (TARGET_MODE_EXIT): Ditto. (TARGET_MODE_PRIORITY): Ditto. * config/riscv/riscv.h (OPTIMIZE_MODE_SWITCHING): Ditto. (NUM_MODES_FOR_MODE_SWITCHING): Ditto. * config/riscv/riscv.md: Add csrwvxrm. * config/riscv/vector.md (rnu,rne,rdn,rod,none): New attribute. (vxrmsi): New pattern. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/vxrm-10.c: New test. * gcc.target/riscv/rvv/base/vxrm-6.c: New test. * gcc.target/riscv/rvv/base/vxrm-7.c: New test. * gcc.target/riscv/rvv/base/vxrm-8.c: New test. * gcc.target/riscv/rvv/base/vxrm-9.c: New test.

auto-generated/api-testing/vaadd.c

This patch-set wants to model rounding mode for the fixed-point intrinsics of the RVV C intrinsics. The specification PR: [riscv-non-isa/rvv-intrinsic-doc#222](riscv-non-isa/rvv-intrinsic-doc#222) The 3 patches is a proof-of-concept with a bottom-up approach Going from machine instruction to LLVM intrinsics, then to the C intrinsics. The 3 patches applies the rounding mode control on the `vaadd` instruction. Proceeding patches will extend the change to all other fixed-point computations. --- This is the 1st commit of the patch-set. This patch gives a name to the machine instruction that writes an immediate into the CSR `vxrm`. Reviewed By: craig.topper Differential Revision: https://reviews.llvm.org/D151395