RFR: 8328404: RISC-V: Fix potential crash in C2_MacroAssembler::arrays_equals

[Fei Yang]

On Tue, 19 Mar 2024 03:44:10 GMT, Gui Cao <gcao at openjdk.org> wrote:

> Hi, The current behavior of C2_MacroAssembler::arrays_equals always load longword before comparison.
> When array[0] is aligned to 32-bit (especially after JDK-8139457 which tries to relax alignment
> of array elements), the last longword load will exceed the array limit and may touch the next
> word beyond object layout in heap memory. So this should bear a similar problem as JDK-8328138.
> 
> Proposed fix changes this behavior and aligns with handling in C2_MacroAssembler::string_equals,
> which will check the number of remaining array elements before loading the next longword.
> No obvious changes witnessed from the JMH numbers or benchmarks like SPECjbb2015.
> 
> Patch also removed the AvoidUnalignedAccesses check in C2_MacroAssembler::string_equals as we
> don't see extra performance gain when setting AvoidUnalignedAccesses to false when testing the
> JMH tests or benchmarks like SPECjbb2015 on three popular RISC-V hardware platforms. We can
> consider adding it back if it turns out to be usefull on future new hardwares.
> 
> 
> ### Correctness test:
> - [x]  Run tier1-3, hotspot:tier4 tests on LicheePi 4A (release)
> - [x]  Run tier1-3, hotspot:tier4 tests on SOPHON SG2042 (release)
> 
> 
> ### JMH test:
> 
> #### 1. test/micro/org/openjdk/bench/java/util/ArraysEquals.java
> 1. SiFive unmatched
> 
> Before:
> Benchmark                            Mode  Cnt   Score   Error  Units
> ArraysEquals.testByteFalseBeginning  avgt   12   37.804 ± 7.292  ns/op
> ArraysEquals.testByteFalseEnd        avgt   12   77.972 ± 3.208  ns/op
> ArraysEquals.testByteFalseMid        avgt   12   54.427 ± 6.436  ns/op
> ArraysEquals.testByteTrue            avgt   12   75.121 ± 5.172  ns/op
> ArraysEquals.testCharFalseBeginning  avgt   12   42.486 ± 6.526  ns/op
> ArraysEquals.testCharFalseEnd        avgt   12  122.208 ± 2.533  ns/op
> ArraysEquals.testCharFalseMid        avgt   12   83.891 ± 3.680  ns/op
> ArraysEquals.testCharTrue            avgt   12  122.096 ± 5.519  ns/op
> 
> After:
> Benchmark                            Mode  Cnt   Score   Error  Units
> ArraysEquals.testByteFalseBeginning  avgt   12   32.638 ± 7.279  ns/op
> ArraysEquals.testByteFalseEnd        avgt   12   73.013 ± 8.081  ns/op
> ArraysEquals.testByteFalseMid        avgt   12   43.619 ± 6.104  ns/op
> ArraysEquals.testByteTrue            avgt   12   83.044 ± 8.207  ns/op
> ArraysEquals.testCharFalseBeginning  avgt   12   39.154 ± 5.233  ns/op
> ArraysEquals.testCharFalseEnd        avgt   12  122.072 ± 7.784  ns/op
> ArraysEquals.testCharFalseMid        avgt   12   67.831 ± 9.218  ns/op
> Ar...

Looks fine. In fact, array[0] could have an alignement of 32-bit after JDK-8139457 when running with -XX:-UseCompressedClassPointers. In this case, we have base_offset = 20 (bytes). It will also an issue when we add support for lilliput on riscv some day, in which case we will have base_offset = 12 (bytes).

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

Marked as reviewed by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18370#pullrequestreview-1947866478