RFR: 8309502: RISC-V: String.indexOf intrinsic may produce misaligned memory loads [v3]

[Fei Yang]

On Wed, 7 Jun 2023 18:20:26 GMT, Vladimir Kempik <vkempik at openjdk.org> wrote:

>> Please review this attempt to remove misaligned loads in String.indexOf intrinsic on RISC-V
>> 
>> Initialy found these misaligned loads when profiling finagle-http test from renaissance suite.
>> The majority of trp_lam events (about 66k per finagle-http round) came at line 706 (https://github.com/openjdk/jdk/pull/14320/files#diff-35eb1d2f1e2f0514dd46bd7fbad49ff2c87703d5a3041a6433956df00a3fe6e6L706)
>> The other two produced about 100 events combined.
>> Later I've found this can partially be reproduced with StringIndexOf.advancedWithMediumSub.
>> Numbers on hifive before and after applying the patch:
>> 
>> 
>> Benchmark                                                  Mode  Cnt       Score      Error  Units
>> StringIndexOf.advancedWithMediumSub                        avgt   25   47031.406 ±  144.005  ns/op
>> 
>> 
>> After:
>> 
>> Benchmark                                                 Mode  Cnt       Score     Error  Units
>> StringIndexOf.advancedWithMediumSub                       avgt   25    4256.830 ±  23.075  ns/op
>> 
>> 
>> Testing: tier1/tier2 is clean on hifive.
>
> Vladimir Kempik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix misaligned access in DO4

@VladimirKempik : Thanks for the update. Would you mind one more tweak? Since `needle_chr_shift` and `haystack_chr_shift` could be 0 for the L case, I think we should guard the shift instructions at https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L634, https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L637, https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L679, https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L700, https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L724, https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L775 with conditions `if (needle_chr_shift)` or `if (haystack_chr_shift)`.

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

> 492:   bne(tmp3, skipch, BMSKIP); // if not equal, skipch is bad char
> 493:   add(result, haystack, isLL ? nlen_tmp : ch2);
> 494:   load_long_misaligned(ch2, Address(result), ch1); // can use ch1 as tmpreg here as it will be trashed on next mv command anyway

Suggestion for comment: // can use ch1 as temp register here as it will be trashed by next mv anyway

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

> 686:       (this->*load_4chr)(ch2, Address(tmp3), noreg);
> 687:       if (isLL)
> 688:       {

Suggestion for coding style: `if (isLL) {`

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

Changes requested by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14320#pullrequestreview-1468875975
PR Review Comment: https://git.openjdk.org/jdk/pull/14320#discussion_r1222369153
PR Review Comment: https://git.openjdk.org/jdk/pull/14320#discussion_r1222369603