On Tue, 16 May 2023 14:44:25 GMT, Richard Reingruber <[email protected]> wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add NONZERO check for downcall_stub_address_offset_in_bytes().
>
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 28:
>
>> 26: package jdk.internal.foreign.abi.ppc64;
>> 27:
>> 28: import java.lang.foreign.AddressLayout;
>
> Imports are not grouped and ordered alphabetically.
> (Very much as the aarch64 version)
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 73:
>
>> 71: public static final int MAX_FLOAT_REGISTER_ARGUMENTS = 13;
>> 72:
>> 73: // This is derived from the 64-Bit ELF V2 ABI spec, restricted to
>> what's
>
> The comment says ABI V2 but the code seems to handle V1 too.
It's derived from ABI v2, but v1 is compatible. Added that to the comment.
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 158:
>
>> 156: class StorageCalculator {
>> 157: private final boolean forArguments;
>> 158: private boolean forVarArgs = false;
>
> Seems to be not used.
I had kept it in case another PPC64 OS would need it, but I guess it's
unlikely. So, I just removed it. Could get added back easily if needed.
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 221:
>
>> 219: // !useABIv2() && layout.byteSize() > 8 &&
>> layout.byteSize() % 8 != 0
>> 220:
>> 221: // Allocate individual fields as gp slots (regs and stack).
>
> You explained to me, it's not individual (struct) fields that are handled
> here. Looks like registers and 8 byte stack slots are allocated to completely
> cover the struct. Would be good if you could change the comment and names in
> the code to better reflect this.
Adapted to aarch64 implementation (using `MAX_COPY_SIZE`).
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/linux/LinuxPPC64leLinker.java
> line 41:
>
>> 39:
>> 40: public static LinuxPPC64leLinker getInstance() {
>> 41: if (instance == null) {
>
> Other platforms optimized this to return a constant (probably after you
> forked off the port).
Good catch. Adapted.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1199271594
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1199271941
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1199272912
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1199273422
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1199273918
