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

[Fei Yang]

On Mon, 24 Feb 2025 15:11:29 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 2

Do you mind several more tweaks? Looks good otherwise. Thanks.

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

> 1387:                                                   const bool isLL, Register cnt1, Register cnt2,
> 1388:                                                   Register tmp1, Register tmp2, Register tmp3,
> 1389:                                                   const int STUB_THRESHOLD, Label *DONE, Label *STUB) {

Personally I prefer to put together STUB_THRESHOLD and STUB, like : `const int STUB_THRESHOLD, Label *STUB, Label *DONE)`. Similar for `C2_MacroAssembler::string_compare_long_different_encoding`.

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

> 1432:   add(str1, str1, cnt2);
> 1433:   sub(cnt2, zr, cnt2);
> 1434: 

Unnecessary new line.

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

> 1478: void C2_MacroAssembler::string_compare_long_different_encoding(Register result, Register str1, Register str2,
> 1479:                                                bool isLU, Register cnt1, Register cnt2,
> 1480:                                                Register tmpL, Register tmpU, Register tmp3,

There is a naming inconsistency for the params. The prototype in the header file says `Register tmp1, Register tmp2, Register tmp3,`. I think we can still use that naming here and create aliases like `tmpL` and `tmpU` as needed in this routine. Like: `Register tmpL = tmp1; Register tmpU = tmp2;`

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

> 1503:   sub(cnt2, zr, cnt2);
> 1504:   addi(cnt1, cnt1, 4);
> 1505: 

Unnecessary new line.

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

> 1594:   // Load 4 bytes once to compare for alignment before main loop. Note that this
> 1595:   // is only possible for LL/UU case. We need to resort to load_long_misaligned
> 1596:   // for both LU and UL cases.

This code comment needs to be moved to the proper place.

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2706:

> 2704:     __ bltz(cnt2, TAIL);
> 2705:     __ bind(SMALL_LOOP);
> 2706:       compare_string_16_bytes_same(DIFF, DIFF2, result, str1, cnt1, str2, tmp1, tmp2, tmp4, tmp5);

Witnessed quite some implicit register dependencies here. And there is only one callsite of `compare_string_16_bytes_same`. Seems more readable if we inline its code here.

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

PR Review: https://git.openjdk.org/jdk/pull/23633#pullrequestreview-2638980565
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1968812819
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1968789299
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1968741111
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1968789215
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1968729207
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1968726664