RFR: 8139457: Array bases are aligned at HeapWord granularity [v33]

[Emanuel Peter]

On Tue, 15 Aug 2023 14:14:01 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> I was looking at this again, and my review is NOT a full review.  I only reviewed the metadata changes.
>
> I think it'd be a good time to move this forward and get it into jdk22 asap, as preparation for the upcoming Lilliput. As far as I can tell, this PR is in good shape now. @coleenp and others, what do you think?

@rkennke I have not yet had a chance to look at the code in detail here. But I have a more general concern.

In SuperWord, we auto-vectorize loops over arrays. In some conditions, we want to have the vector loads/stores aligned. On some machines this is done mostly for performance (e.g. cache line alignment). On other machines, this is a strict requirement (otherwise we either get a SIGBUS or wrong results). The flag `-XX:+AlignVector` can be set to force this strict alignment, and some platforms have it always enabled, see `Matcher::misaligned_vectors_ok() = false`.

The current alignment logic in SuperWord is subtly broken, and I am working on a fix [JDK-8310190](https://bugs.openjdk.org/browse/JDK-8310190).
But what I know is that we do the alignment checks statically, assuming that the array base is aligned. I think the assumption is that it is aligned with `ObjectAlignmentInBytes = 8` bytes. See for example in `SuperWord::align_initial_loop_index`:
https://github.com/openjdk/jdk/blob/d46f0fb31888db75f5b2b78a162fec16dfc5d0d9/src/hotspot/share/opto/superword.cpp#L3736-L3738

Some platforms need 4 byte or 8 byte alignment:
https://github.com/openjdk/jdk/blob/d46f0fb31888db75f5b2b78a162fec16dfc5d0d9/src/hotspot/cpu/ppc/matcher_ppc.hpp#L40-L44

As far as I understand, some (if not all arm32) require 8 byte alignment (@fg1417 is this correct?).
https://github.com/openjdk/jdk/blob/d46f0fb31888db75f5b2b78a162fec16dfc5d0d9/src/hotspot/cpu/arm/matcher_arm.hpp#L40-L43

The alignment checks are performed statically, by analyzing the pointer of the memory load/store into this form:
https://github.com/openjdk/jdk/blob/d46f0fb31888db75f5b2b78a162fec16dfc5d0d9/src/hotspot/share/opto/superword.cpp#L785

As far as I understand, the `base` is always assumed to be aligned. We only check that adding the other components to the base would keep the address aligned.

Hence this change here makes me a bit nervous. The problem is that we currently do not have very good tests. I'm working on improving tests and verification with [JDK-8310190](https://bugs.openjdk.org/browse/JDK-8310190).

Are you aware of the possible impact on SuperWord alignment? Maybe it is really only going to be an issue on arm32. What do you think @fg1417 ?

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

PR Comment: https://git.openjdk.org/jdk/pull/11044#issuecomment-1680149988