On Fri, 24 Feb 2023 07:17:30 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Some more remarks about other issues: >> - Uploaded my simple reproducer to >> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017) >> - Using oversized load / stores is problematic. Don't forget that OpenJDK >> still supports Big Endian platforms (AIX, s390x). >> - The result of `NativeCallingConvention::calling_convention` is interpreted >> as size, but it returns the max offset. That's off by one slot. Should I >> file a bug for that? (PPC64 is not affected because it doesn't use the >> result.) >> - Since the membar on the return path was mentioned: I think it would be >> good to enable UseSystemMemoryBarrier by default on operating systems which >> support it. Maybe we should discuss this with @robehn. > >> * Uploaded my simple reproducer to >> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017) > > Thanks! > >> * Using oversized load / stores is problematic. Don't forget that >> OpenJDK still supports Big Endian platforms (AIX, s390x). > > You're right. I realized that it's also problematic for heap segments, for > which we can't do oversized accesses. I am working on another solution that > splits up the loads/stores into power-of-two sized chunks: > https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:OOB > That patch is just a POC at this point though. Also, I don't think it works > for BE at the moment (need to flip the offset for BE, I think. Just like we > do in Unsafe). > >> * The result of `NativeCallingConvention::calling_convention` is >> interpreted as size, but it returns the max offset. That's off by one slot. >> Should I file a bug for that? (PPC64 is not affected because it doesn't use >> the result.) > > I'm not sure there's an issue there. Note that the 'max offset' is computed > as `reg.offset() + reg.stack_size()`, so that should get us the size we need > to allocate for the stack arguments. (e.g. 2 ints being passed at offset 0 > and 4, would make max offset 4 + 4 = 8, which gives the size needed for the 2 > ints). Computing the max offset instead of just summing the sizes of the > stack arguments is needed since stack arguments can be sparsely placed in > some cases on Mac/AArch64. > >> * Since the membar on the return path was mentioned: I think it would be >> good to enable UseSystemMemoryBarrier by default on operating systems which >> support it. Maybe we should discuss this with @robehn. > > ~I don't think we've done that much testing with UseSystemMemoryBarrier since > it was added~. I'm a bit nervous about turning it on by default since it's > currently also used for JNI. Let's see what Robbin thinks. @JornVernee: Thanks a lot for your detailed review! I have quite a few TODOs which include: - Include my tests for the HFA corner cases. - Try to improve handling of the overlapping registers as you suggested. - Check nesting of HFA. There will surely be more when looking into Big Endian support after merging with your recent work on https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:OOB We should get rid of oversized accesses on PPC64, too. Thanks for sharing your plans to intrisify `linkToNative` in C2 later. I guess we should do more preparation work on all platforms when that gets addressed. ------------- PR: https://git.openjdk.org/jdk/pull/12708
