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

[Axel Boldt-Christmas]

On Wed, 8 May 2024 07:18:06 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Unify impls for +/-CCP
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5558:
> 
>> 5556:   int start_offset = length_is_8aligned ? length_offset : klass_offset;
>> 5557:   assert(is_aligned(start_offset, BytesPerWord), "start offset must be 8-byte-aligned");
>> 5558:   int extra_length = length_is_8aligned ? base_offset - length_offset : base_offset - klass_offset;
> 
> Some of these asserts and comments seem a little bit out of place, inaccurate and/or incomplete with the new change to include the klass in the comparison. 
> 
> Suggestion:
> 
>   // When the length offset is not aligned to 8 bytes, 
>   // then we let the compare loop include the klass
>   // and length, otherwise start from the length.
>   bool length_is_8aligned = is_aligned(length_offset, BytesPerWord);
>   int start_offset = length_is_8aligned ? length_offset : klass_offset;
>   assert(is_aligned(start_offset, BytesPerWord), "start offset must be 8-byte-aligned");
>   int extra_length =  base_offset - start_offset;
>   assert(!length_is_8aligned || extra_length == BytesPerInt,
>          "no padding between length and base");
>   assert(length_is_8aligned || extra_length == BytesPerWord,
>          "no padding between klass, length and base");

Also right now the `length_is_8aligned` very much now just look like a `!oopDesc::has_klass_gap()` check.

The validity of choosing to start from the klass when `!length_is_8aligned` is that we must then be using `UseCompressedClassPointers /* && !UseCompactObjectHeaders  for Lilliput */`. Until Lilliput starting from the Klass is valid for both `+UseCompressedClassPointers` and `-UseCompressedClassPointers` as the klass, length and base will always be tightly packed (no padding) for byte and char type arrays. 

In all modes what we effectively do is `int start_offset = align_down(length_offset, BytesPerWord )`. Not sure if the intent gets clearer then. Something along the lines of:
Suggestion:

  // When the length offset is not aligned to 8 bytes,
  // then we align it down, this is valid as the new
  // offset will always be the klass which is the same
  // for type arrays.
  int start_offset = align_down(length_offset, BytesPerWord);
  int extra_length =  base_offset - start_offset;
  assert(start_offset == length_offset || start_offset == klass_offset, 
         "start offset must be 8-byte-aligned or be the klass offset");
  assert(base_offset != start_offset, "must include the length field");

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1593537114