[foreign-memaccess+abi] RFR: 8299911: Refactor struct handling in AArch64 CallArranger

[Jorn Vernee]

On Wed, 11 Jan 2023 12:29:21 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Refactor struct handling in AArch64 CallArranger.
>> 
>> Instead of having 6 very similar looking while loops, I've consolidated the storage allocation code for structs into a single helper method (`StorageCalculator::structStorages`). This should make it much easier to see which logic is being used for a particular case.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 302:
> 
>> 300:                 if (isFieldWise) {
>> 301:                     // fields only, no padding
>> 302:                     copyLayout = (ValueLayout) layout.memberLayouts().get(i);
> 
> Doesn't this contradict the comment? E.g. if we only copy fields, then we should skip padding layouts here (as they can be members of a group layout) ?

The cast would fail if we ran into a padding layout, so there is an implicit assumption that we will not see any padding. I added the comment there to emphasize that. If we could have padding at this point, the offset computation we do later would also be incorrect.

> src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 306:
> 
>> 304:                     // chunk-wise copy
>> 305:                     long copySize = Math.min(layout.byteSize() - offset, MAX_COPY_SIZE);
>> 306:                     boolean useFloat = false; // never use float for chunk-wise copies
> 
> this local seems unnecessary?

The local is there to give a name to the magic `false` value, and it also makes room for putting a comment. I preferred doing this to the alternative:

    copyLayout = SharedUtils.primitiveLayoutForSize(copySize, /* useFloat = */ false); // never use float for chunk-wise copies

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

PR: https://git.openjdk.org/panama-foreign/pull/765