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

[Hamlin Li]

On Mon, 24 Feb 2025 03:17:03 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Hamlin Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision:
>> 
>>  - fix UL and test
>>  - Merge branch 'master' into refactor-string-compare
>>  - minor
>>  - fix temp registers; move code
>>  - blank lines
>>  - simplify
>>  - clean
>>  - merge UL and LU
>>  - move to functions
>>  - move alignment code of LL&UU down from common code path
>>  - ... and 1 more: https://git.openjdk.org/jdk/compare/e85abfd8...4f5ae272
>
> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1605:
> 
>> 1603:       string_compare_long_same_encoding(result, str1, str2,
>> 1604:                                 cnt1, cnt2, tmp1, tmp2, tmp3,
>> 1605:                                 isLL, isLL ? base_offset1 : base_offset2, minCharsInWord,
> 
> `base_offset` is only used by the two new assembler subroutines, so it's more reasonable to calculate there. And `minCharsInWord` is simple and I think this param could also be saved. This way the argument list will be shorter.

OK, will fix.

> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1611:
> 
>> 1609:                              isLU ? str1 : str2,
>> 1610:                              isLU ? str2 : str1,
>> 1611:                              isUL,
> 
> Both `isLU` and `isUL` are used to prepare the params. Personally, I prefer to keep it simple and only use `isLU` here. Using `isLU` will also be kind of consistent with the order of the second and third params of this subroutine (renamed param `isUL` to `IsLU`):
> 
> void C2_MacroAssembler::string_compare_long_different_encoding(Register result, Register strL, Register strU,
>                                                bool isLU, Register cnt1, Register cnt2,
> 
> 
> --------
> PS: Why not pass `str1` and `str1` directly like you do for `string_compare_long_same_encoding`? We can distinuish `strL` and and `strU` in `string_compare_long_different_encoding` with param `isLU`. Seems to me that it will be simpler at the interface level then.

OK, will fix.

> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1618:
> 
>> 1616:   }
>> 1617: 
>> 1618:   bind(STUB);
> 
> Consider moving this STUB generation code to the new subroutines at the same time.

It's better to keep it as it is, they are different logics, and if moving it then the 2 new subroutines will need to access the SHORT_* Labels by passing more parameters.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1967323976
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1967324194
PR Review Comment: https://git.openjdk.org/jdk/pull/23633#discussion_r1967326015