RFR: 8347489: RISC-V: Misaligned memory access with COH [v4]

[Hamlin Li]

On Wed, 15 Jan 2025 08:23:04 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Hi, please consider this change.
>> 
>> We have different base_offset for T_BYTE/T_CHAR (4-byte instead of 8-byte aligned) with COH. This causes misaligned memory accesses for several instrinsics like String.Compare or String.Equals. The reason is that we assume 8-byte alignment and process one 8-byte word starting at the first array element for each iteration in the main loop. As a result, we have performance regressions on platforms with slow misaligned memory accesses like Unmatched and Premier P550 SBCs.
>> 
>> PS: Same issue is there even without COH. base_offset for T_BYTE/T_CHAR is 20 (thus 4-byte aligned) when `UseCompressedClassPointers` is disabled in this case.
>> 
>> Correctness test on linux-riscv64:
>> - [x] tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders") (release)
>> - [x] tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:-UseCompactObjectHeaders") (release)
>> - [x] hotspot:tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders") (fastdebug)
>> - [x] hotspot:tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:-UseCompactObjectHeaders") (fastdebug)
>> 
>> Performance test on Premier P550 (-XX:+UseParallelGC -XX:+AlwaysPreTouch -Xms8g -Xmx8g):
>> 
>> 1. SPECjbb2005 Score Without Patch
>> 1.1 -XX:-UseCompactObjectHeaders: 32666
>> 1.2 -XX:+UseCompactObjectHeaders: 27610
>> 
>> 2. SPECjbb2005 Score With Patch
>> 2.1 -XX:-UseCompactObjectHeaders: 32820
>> 2.2 -XX:+UseCompactObjectHeaders: 34179
>
> Fei Yang 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 six additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8347489
>  - Comment
>  - Fix assertions
>  - Add assertions
>  - Comment
>  - 8347489: RISC-V: Misaligned memory access with COH

Some comments.

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

> 1416:   assert((base_offset1 % (UseCompactObjectHeaders ? 4 :
> 1417:                           (UseCompressedClassPointers ? 8 : 4))) == 0, "Must be");
> 1418:   assert((base_offset2 % (UseCompactObjectHeaders ? 4 :

should it be something like `assert((base_offset1 == base_offset2));` ?

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

> 2472: 
> 2473:     // strL is 8-byte aligned
> 2474:     __ ld(tmpLval, Address(strL));

This should also be a `load_long_misaligned`?

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

> 2510:     assert((base_offset1 % (UseCompactObjectHeaders ? 4 :
> 2511:                             (UseCompressedClassPointers ? 8 : 4))) == 0, "Must be");
> 2512:     assert((base_offset2 % (UseCompactObjectHeaders ? 4 :

should it be something like `assert((base_offset1 == base_offset2));` ?

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

> 2529:              tmpL = isLU ? tmp1 : tmp2; // where to keep L for comparison
> 2530: 
> 2531:     if ((base_offset1 % 8) == 0) {

The condition should be `((base_offset1 % 8) != 0)`? Seems we try to make the `strL` aligned in the main loop below.

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

PR Review: https://git.openjdk.org/jdk/pull/23053#pullrequestreview-2553015865
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1916808262
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1916811937
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1916808497
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1916818899