RFR: 8347489: RISC-V: Misaligned memory access with COH

[Hamlin Li]

On Sun, 12 Jan 2025 03:45:45 GMT, Fei Yang <fyang at openjdk.org> wrote:

> Hi, please consider this change.
> 
> We have different base_offset (4 bytes instead of 8 bytes 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.
> 
> 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

Thanks for the patch.
Can you post the performance data in description?
And some minor comments.

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

> 1413:   int base_offset1 = arrayOopDesc::base_offset_in_bytes(T_BYTE);
> 1414:   int base_offset2 = arrayOopDesc::base_offset_in_bytes(T_CHAR);
> 1415: 

An assert of either 4 || 8 would be helpful here?

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

> 1437:     beq(str1, str2, DONE);
> 1438:     int base_offset = isLL ? base_offset1 : base_offset2;
> 1439:     if ((base_offset % 8) != 0) {

If `AvoidUnalignedAccesses == false`, do we still need this piece of code?

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

> 1691: 
> 1692:   // Load 4 bytes once to compare for alignment before main loop.
> 1693:   if ((base_offset % 8) != 0) {

similar comment here.

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

PR Review: https://git.openjdk.org/jdk/pull/23053#pullrequestreview-2546113978
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1912912442
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1912897443
PR Review Comment: https://git.openjdk.org/jdk/pull/23053#discussion_r1912900082