Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 14:30:06 GMT, Maurizio Cimadamore
wrote:
>> @TheRealMDoerr We've been discussing the shifts in order to wrap our heads
>> around it, and we've ended up with this diagram in order to try and
>> visualize what happens:
>>
>> Let's say we have a struct with 3 ints:
>>
>>
>> struct Foo {
>> int x;
>> int y;
>> int z;
>> };
>>
>>
>> If this struct is passed as an argument, then the load of the second 'half'
>> of the struct would look like this:
>>
>>
>> offset : 0 32 . 64 . 96 128
>> values : ||| (can't touch bits
>> 96..128)
>> Load int : V++
>> | |
>> ++|
>>VV
>> In register: | (MSBs are 0)
>> Shift left : | (LSBs are zero)
>> Write long : V V
>> Result : |||
>>
>>
>> So, the 'Result' is padded at the tail with zeros.
>>
>> Does that seem right? Does it seem useful to add this diagram as a comment
>> somewhere, for us when we come back to this code a year from now? Thanks
>
>> If this struct is passed as an argument, then the load of the second 'half'
>> of the struct would look like this:
>
> It would perhaps be cleaner if in the MSB/LSB comments we said:
>
> LSBs are zzz...z
> LSBs are 000...0
>
> (e.g. avoid to refer to MSBs in the first, since those bytes are not exactly
> zero, they are the padding bytes)
> @mcimadamore: May I ask you or somebody else from the Panama team to provide
> a 2nd review? This PR requires Panama knowledge, not really PPC knowledge.
Sorry - I have already reviewed it - but didn't approve as I was waiting for
@JornVernee to chime in. Now he has, and I will add my approval as well.
-
PR Comment: https://git.openjdk.org/jdk/pull/15417#issuecomment-1707067130
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 14:30:06 GMT, Maurizio Cimadamore
wrote:
>> @TheRealMDoerr We've been discussing the shifts in order to wrap our heads
>> around it, and we've ended up with this diagram in order to try and
>> visualize what happens:
>>
>> Let's say we have a struct with 3 ints:
>>
>>
>> struct Foo {
>> int x;
>> int y;
>> int z;
>> };
>>
>>
>> If this struct is passed as an argument, then the load of the second 'half'
>> of the struct would look like this:
>>
>>
>> offset : 0 32 . 64 . 96 128
>> values : ||| (can't touch bits
>> 96..128)
>> Load int : V++
>> | |
>> ++|
>>VV
>> In register: | (MSBs are 0)
>> Shift left : | (LSBs are zero)
>> Write long : V V
>> Result : |||
>>
>>
>> So, the 'Result' is padded at the tail with zeros.
>>
>> Does that seem right? Does it seem useful to add this diagram as a comment
>> somewhere, for us when we come back to this code a year from now? Thanks
>
>> If this struct is passed as an argument, then the load of the second 'half'
>> of the struct would look like this:
>
> It would perhaps be cleaner if in the MSB/LSB comments we said:
>
> LSBs are zzz...z
> LSBs are 000...0
>
> (e.g. avoid to refer to MSBs in the first, since those bytes are not exactly
> zero, they are the padding bytes)
@mcimadamore: May I ask you or somebody else from the Panama team to provide a
2nd review? This PR requires Panama knowledge, not really PPC knowledge.
-
PR Comment: https://git.openjdk.org/jdk/pull/15417#issuecomment-1706217722
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 14:50:37 GMT, Martin Doerr wrote: >>> as it would no longer need an input type? >> >> Yes. Then both shift ops would always operate on `long`. > > I've changed it. Note that I need many more conversions because buffer > load/store also use subtypes of `int`. Please take a look at my updated > version (after commit number 5). Thanks, your changes look great. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1315040087
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 14:33:08 GMT, Jorn Vernee wrote: >>> I think it's worth it in order to have a cleaner contract for the shift >>> ops, should we want to use them for anything else in the future, but also >>> just to make them easier to understand for future readers. >> >> I agree that having a cleaner contract for the shift binding would prove >> useful in the long run. If we do that, we can also simplify the binding >> itself, as it would no longer need an input type? > >> as it would no longer need an input type? > > Yes. Then both shift ops would always operate on `long`. I've changed it. Note that I need many more conversions because buffer load/store also use subtypes of `int`. Please take a look at my updated version (after commit number 5). - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1315032737
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 14:27:27 GMT, Maurizio Cimadamore wrote: > as it would no longer need an input type? Yes. Then both shift ops would always operate on `long`. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1315016599
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 14:18:44 GMT, Jorn Vernee wrote: > If this struct is passed as an argument, then the load of the second 'half' > of the struct would look like this: It would perhaps be cleaner if in the MSB/LSB comments we said: LSBs are zzz...z LSBs are 000...0 (e.g. avoid to refer to MSBs in the first, since those bytes are not exactly zero, they are the padding bytes) > I think it's worth it in order to have a cleaner contract for the shift ops, > should we want to use them for anything else in the future, but also just to > make them easier to understand for future readers. I agree that having a cleaner contract for the shift binding would prove useful in the long run. If we do that, we can also simplify the binding itself, as it would no longer need an input type? - PR Comment: https://git.openjdk.org/jdk/pull/15417#issuecomment-1705366497 PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1315010943
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 12:20:52 GMT, Martin Doerr wrote:
>> Sorry for the delay, I've been on vacation.
>
>> Sorry for the delay, I've been on vacation.
>
> No problem. Hope you had a good time! Thanks for your feedback.
@TheRealMDoerr We've been discussing the shifts in order to wrap our heads
around it, and we've ended up with this diagram in order to try and visualize
what happens:
Let's say we have a struct with 3 ints:
struct Foo {
int x;
int y;
int z;
};
If this struct is passed as an argument, then the load of the second 'half' of
the struct would look like this:
offset : 0 32 . 64 . 96 128
values : ||| (can't touch bits
96..128)
Load int : V++
| |
++|
VV
In register: | (MSBs are 0)
Shift left : | (LSBs are zero)
Write long : V V
Result : |||
So, the 'Result' is padded at the tail with zeros.
Does that seem right? Does it seem useful to add this diagram as a comment
somewhere, for us when we come back to this code a year from now? Thanks
-
PR Comment: https://git.openjdk.org/jdk/pull/15417#issuecomment-1705350592
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 12:19:42 GMT, Martin Doerr wrote: >> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 398: >> >>> 396: bindings.add(Binding.cast(type, int.class)); >>> 397: type = int.class; >>> 398: } >> >> Part of the casts are handled here with explicit cast bindings, but the >> widening from int -> long, and narrowing from long -> int are handled >> implicitly as part of the ShiftLeft implementation. I'd much prefer if all >> the type conversions are handled with explicit cast bindings. This would >> also semantically simplify the shift operator, since it would just handle >> the actual shifting. > > I guess we would need to add additional bindings for that? Is is worth adding > more just for a big endian corner case? Or can that be done with the existing > ones? A `Cast` case for int -> long and long -> int would need to be added. Given the existing setup, that should only be a few lines of code for each. (See e.g. for int -> long https://github.com/openjdk/jdk/compare/master...JornVernee:jdk:I2L). I don't think the cost is that high. > Is is worth adding more just for a big endian corner case? I think it's worth it in order to have a cleaner contract for the shift ops, should we want to use them for anything else in the future, but also just to make them easier to understand for future readers. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314892030
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 07:26:36 GMT, Jorn Vernee wrote: > Sorry for the delay, I've been on vacation. No problem. Hope you had a good time! Thanks for your feedback. > src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 398: > >> 396: bindings.add(Binding.cast(type, int.class)); >> 397: type = int.class; >> 398: } > > Part of the casts are handled here with explicit cast bindings, but the > widening from int -> long, and narrowing from long -> int are handled > implicitly as part of the ShiftLeft implementation. I'd much prefer if all > the type conversions are handled with explicit cast bindings. This would also > semantically simplify the shift operator, since it would just handle the > actual shifting. I guess we would need to add additional bindings for that? Is is worth adding more just for a big endian corner case? Or can that be done with the existing ones? - PR Comment: https://git.openjdk.org/jdk/pull/15417#issuecomment-1705174117 PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314874972
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Mon, 4 Sep 2023 07:06:39 GMT, Jorn Vernee wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unnecessary imports.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 695:
>
>> 693: * Negative [shiftAmount] shifts right and converts to int if
>> needed.
>> 694: */
>> 695: record ShiftLeft(int shiftAmount, Class type) implements Binding
>> {
>
> Please split this into 2 binding operators: one for ShiftLeft and another for
> (arithmetic) ShiftRight, rather than mixing the 2 implementations into a
> single operator.
>
> For the right shift you could then also use a positive shift amount, and
> avoid the double negation that's needed right now.
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 703:
>
>> 701: if (last != ((type == long.class) ? long.class :
>> int.class))
>> 702: throw new IllegalArgumentException(
>> 703: String.format("Invalid operand type: %s.
>> integral type expected", last));
>
> Why not use `SharedUtils.checkType` here?
>
> Suggestion:
>
> SharedUtils.checkType(last, type);
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 711:
>
>> 709: stack.push(type == long.class ? long.class : int.class);
>> 710: } else
>> 711: throw new IllegalArgumentException("shiftAmount 0 not
>> supported");
>
> Please handle this validation in the static `shiftLeft` method. That's also
> where we do validation for other binding ops
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 737:
>
>> 735: cb.i2l();
>> 736: typeStack.pop();
>> 737: typeStack.push(long.class);
>
> Please use `popType` and `pushType` instead of manipulating the type stack
> manually. The former also does some verification. This should happen along
> every control path. Even if the binding is 1-to-1 (like this one) and doesn't
> change the type of the value, this would validate that the input type is
> correct, and make sure that any bugs are caught early.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314872201
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314871165
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314871596
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314871489
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Fri, 25 Aug 2023 09:24:15 GMT, Maurizio Cimadamore wrote: >> The ABI says: "An aggregate or union smaller than one doubleword in size is >> padded so that it appears in the least significant bits of the doubleword. >> All others are padded, if necessary, at their tail." >> [https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#PARAM-PASS]. >> I have written examples which pass 9 and 15 Bytes. >> In the first case, we need to get 0x0001020304050607 in the first argument >> and 0x08XX into the second argument (X is "don't care"). Shift >> amount is 7. >> In the second case, we need to get 0x0001020304050607 in the first argument >> and 0x08090a0b0c0d0eXX into the second argument. Shift amount is 1. >> In other words, we need shift amounts between 1 and 7. Stack slots and >> registers are always 64 bit on PPC64. > > Got it - I found these representations: > > https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.7.html#BYTEORDER > > Very helpful. So you have e.g. a `short` value (loaded from somewhere) and > you have to store it on a double-word. Now, if you just stored it at offset > 0, you will write the bits 0-15, which are the "most" significant bits in > big-endian representation. So, it's backwards. I believe FFM will take care > of endianness, so that the bytes 0-7 and 8-15 will be "swapped" when writing > into the double-word (right?) but their base offset (0) is still off, as they > should really start at offset 48. Hence the shift. After looking for a while, I think having the new binding operator is needed. e.g. for stores: we load a 32 bit value from the end of a struct, then the low-order bits of the value needs to be padded with zeros to get a 64 bit register value, leaving the original 32 bit value in the high order bits. This can't be handled by the current cast operator. e.g. if we had an int -> long cast conversion, then in the resulting value the low-order bits would be occupied by the 32 bit value, which is incorrect. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314524955
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
On Fri, 25 Aug 2023 10:59:45 GMT, Martin Doerr wrote:
>> I've found a way to solve the remaining FFI problem on linux PPC64 Big
>> Endian. Large structs (>8 Bytes) which are passed in registers or on stack
>> require shifting the Bytes in the last slot if the size is not a multiple of
>> 8. This PR adds the required functionality to the Java code.
>>
>> Please review and provide feedback. There may be better ways to implement
>> it. I just found one which works and makes the tests pass:
>>
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/foreign 8888 0 0
>>
>>
>>
>> Note: This PR should be considered as preparation work for AIX which also
>> uses ABIv1.
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove unnecessary imports.
Sorry for the delay, I've been on vacation.
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 398:
> 396: bindings.add(Binding.cast(type, int.class));
> 397: type = int.class;
> 398: }
Part of the casts are handled here with explicit cast bindings, but the
widening from int -> long, and narrowing from long -> int are handled
implicitly as part of the ShiftLeft implementation. I'd much prefer if all the
type conversions are handled with explicit cast bindings. This would also
semantically simplify the shift operator, since it would just handle the actual
shifting.
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 695:
> 693: * Negative [shiftAmount] shifts right and converts to int if
> needed.
> 694: */
> 695: record ShiftLeft(int shiftAmount, Class type) implements Binding {
Please split this into 2 binding operators: one for ShiftLeft and another for
(arithmetic) ShiftRight, rather than mixing the 2 implementations into a single
operator.
For the right shift you could then also use a positive shift amount, and avoid
the double negation that's needed right now.
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 703:
> 701: if (last != ((type == long.class) ? long.class :
> int.class))
> 702: throw new IllegalArgumentException(
> 703: String.format("Invalid operand type: %s.
> integral type expected", last));
Why not use `SharedUtils.checkType` here?
Suggestion:
SharedUtils.checkType(last, type);
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 711:
> 709: stack.push(type == long.class ? long.class : int.class);
> 710: } else
> 711: throw new IllegalArgumentException("shiftAmount 0 not
> supported");
Please handle this validation in the static `shiftLeft` method. That's also
where we do validation for other binding ops
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 737:
> 735: cb.i2l();
> 736: typeStack.pop();
> 737: typeStack.push(long.class);
Please use `popType` and `pushType` instead of manipulating the type stack
manually. The former also does some verification. This should happen along
every control path. Even if the binding is 1-to-1 (like this one) and doesn't
change the type of the value, this would validate that the input type is
correct, and make sure that any bugs are caught early.
-
PR Review: https://git.openjdk.org/jdk/pull/15417#pullrequestreview-1595698959
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305681864
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314518216
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305642804
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305668315
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305650457
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
> I've found a way to solve the remaining FFI problem on linux PPC64 Big > Endian. Large structs (>8 Bytes) which are passed in registers or on stack > require shifting the Bytes in the last slot if the size is not a multiple of > 8. This PR adds the required functionality to the Java code. > > Please review and provide feedback. There may be better ways to implement it. > I just found one which works and makes the tests pass: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/foreign 8888 0 0 > > > > Note: This PR should be considered as preparation work for AIX which also > uses ABIv1. Martin Doerr has updated the pull request incrementally with one additional commit since the last revision: Remove unnecessary imports. - Changes: - all: https://git.openjdk.org/jdk/pull/15417/files - new: https://git.openjdk.org/jdk/pull/15417/files/50144b14..430fa018 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15417&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15417&range=01-02 Stats: 5 lines in 3 files changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15417.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15417/head:pull/15417 PR: https://git.openjdk.org/jdk/pull/15417
