RFR: 8331098: [Aarch64] Fix crash in Arrays.equals() intrinsic with -CCP [v4]

[Axel Boldt-Christmas]

On Fri, 26 Apr 2024 11:22:03 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> The implementations of Arrays.equals() in macroAssembler_aarch64.cpp, MacroAssembler::arrays_equals() assumes that the start of arrays is 8-byte-aligned. Since [JDK-8139457](https://bugs.openjdk.org/browse/JDK-8139457) this is no longer the case, at least when running with -CompressedClassPointers (or Lilliput). The effect is that the loops may run over the array end, and if the array is at heap boundary, and that memory is unmapped, then it may crash.
>> 
>> The proposed fix aims to always enter the main loop(s) with an aligned address:
>>  - When the array base is 8-byte-aligned (default, with +CCP), then compare the array lengths separately, then enter the main loop with the array base.
>>  - When the array base is not 8-byte-aligned (-CCP and Lilliput), then enter the loop with the address of the array-length (which is then 8-byte-aligned), and compare array lengths in the main loop, and elide the explicit array lengths comparison.
>> 
>> Testing:
>>  - [x] tier1 (+CCP)
>>  - [x] tier1 (-CCP)
>>  - [x] tier2 (+CCP)
>>  - [x] tier2 (-CCP)
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove extra whitespace

I can review and approve the correctness of the implementation.

However I cannot comment on any performance implications. The whole `!UseSimpleArrayEquals` looks like it was design with (some specific) performance in mind. 

Maybe the bug fix should be done first, and let any performance implications be dealt with separately. Especially since the emitted code should be the same when running with`+UseCompressedClassPointers`.

For me it reads better if the
```c++
    if (extra_length != 0) {
      // [CODE]
    }
    if (is_8aligned) {
      // [CODE]
    }

and 
```c++
    if (is_8aligned) {
      // [CODE]
    }
    if (extra_length != 0) {
      // [CODE]
    }

were just turned into:
```c++
    if (is_8aligned) {
      // [CODE]
    } else {
      assert(extra_length != 0, "maybe even assert this, not sure if needed");
      // [CODE]
    }


Side note on the performance:
There is also [JDK-8328138](https://bugs.openjdk.org/browse/JDK-8328138) / #18292 which proposes another variant of array equals, which seems to be a more optimised version of the simple array equals. I think this speaks even more for just taking in the bug fix without evaluating the performance with `-UseCompressedClassPointers`

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5770:

> 5768:     sub(tmp5, zr, cnt1, LSL, 3 + log_elem_size);
> 5769:     ldr(tmp3, Address(pre(a1, start_offset)));
> 5770:     ldr(tmp4, Address(pre(a2, start_offset)));

Is the use of `pre` intentional? If that is the case why would that be better than just a `reg + offset`? (Given that `a1` and `a2` are not used again)

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

PR Review: https://git.openjdk.org/jdk/pull/18948#pullrequestreview-2040626704
PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1590922510