RFR: 8350095: RISC-V: Refactor string_compare [v4]

[Fei Yang]

On Mon, 24 Feb 2025 10:05:33 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you help to review this patch?
>> 
>> Currently, `string_compare` code is a bit complicated, main reasons include:
>> 1. it has 2 piece of code respectively for LU and UL case, this is not necessary, basically LU and UL behaviour quite similar.
>> 2. it mixed LL/UU and LU/UL case together, better to separate them, as they are quite different from each other.
>> 
>> This is not good for code reading and maintaining.
>> 
>> 
>> So, this patch does following refactoring:
>> 1. merge LU and UL code into one, i.e. remove UL code.
>> 2. seperate the code into 2 methods:  LL/UU and LU/UL.
>> 3. some other misc improvement.
>> 
>> I could do the following refactoring in another following pr, as in this pr I'm just moving code and removing code, it's easier to do it and review it. In particular the first one, as it needs to rewrite the existing code for UL/LU case.
>> 1. move alignment code of `generate_compare_long_string_different_encoding` upwards to `string_compare_long_LU`.
>> 2. make `SHORT_STRING` case simpler.
>> 
>> 
>> 
>> Thanks
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   clean

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1398:

> 1396:   assert((base_offset2 % (UseCompactObjectHeaders ? 4 :
> 1397:                           (UseCompressedClassPointers ? 8 : 4))) == 0, "Must be");
> 1398:   const int base_offset = isLL ? base_offset1 : base_offset2;

Since only one of `base_offset1` and `base_offset2` will be used, better to do:

  const int base_offset = isLL ? arrayOopDesc::base_offset_in_bytes(T_BYTE)
                               : arrayOopDesc::base_offset_in_bytes(T_CHAR);
  assert((base_offset % (UseCompactObjectHeaders ? 4 :
                          (UseCompressedClassPointers ? 8 : 4))) == 0, "Must be");

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1452:

> 1450: 
> 1451:   bne(tmp1, tmp2, DIFFERENCE);
> 1452:   bltz(cnt2, NEXT_WORD);

As these two instructions belong the main loop, I think it's cleaner to add proper indentation for them and group them with other instructions in the loop together.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1526:

> 1524: 
> 1525:   bne(tmpL, tmpU, DIFFERENCE);
> 1526:   bltz(cnt2, NEXT_WORD);

Similar here. Do you mind adding proper indentation for these instructions and group them with other instructions in the main loop together.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1967474451
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1967478342
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1967480109