RFR: 8321003: RISC-V: C2 MulReductionVI [v2]

[Hamlin Li]

On Thu, 13 Feb 2025 02:41:47 GMT, Fei Yang <fyang at openjdk.org> wrote:

> Hi, Some comments after a cursory look. Will have a more closer look later.
> 
> BTW: How should I understand the JMH data? 11170.052 ns/op before compared to 1294.424 ns/op after for ByteMaxVector.MULLanes, but the improvement says 88.40%?

Thanks for having a look.
`Improvement` is calculated by (`master` - `patch`) / `master`, means how much time is saved by new implementation compared to the old one.

> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2961:
> 
>> 2959:                                              BasicType bt, uint vector_length, VectorMask vm) {
>> 2960:   assert(bt == T_BYTE || bt == T_SHORT || bt == T_INT || bt == T_LONG, "unsupported element type");
>> 2961:   uint len = vector_length/type2aelembytes(bt);
> 
> Please put a space before and after the divide operator.

fixed.

> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 3012:
> 
>> 3010:       lui(t0, 0x3f800000); // 1.0f
>> 3011:     } else {
>> 3012:       lui(t0, 0x3ff00000); // 1.0d
> 
> Can you use `mv` instead of `lui`, like other places?

fixed.

> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp line 242:
> 
>> 240:                         VectorMask vm = Assembler::unmasked);
>> 241: 
>> 242:   void reduce_mul_integer_v(Register dst, Register src1, VectorRegister src2,
> 
> Or maybe `reduce_mul_integral_v` which will be consistent in naming with friends like `reduce_integral_v`?

fixed.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23580#issuecomment-2655934207
PR Review Comment: https://git.openjdk.org/jdk/pull/23580#discussion_r1954089593
PR Review Comment: https://git.openjdk.org/jdk/pull/23580#discussion_r1954089906
PR Review Comment: https://git.openjdk.org/jdk/pull/23580#discussion_r1954089415