RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v2]
Jorn Vernee
jvernee at openjdk.org
Fri Jul 7 13:13:06 UTC 2023
On Fri, 7 Jul 2023 10:01:20 GMT, sid8606 <duke at openjdk.org> wrote:
>> Implementation of "Foreign Function & Memory API" for s390x.
>
> sid8606 has updated the pull request incrementally with one additional commit since the last revision:
>
> Address Amit's review comments
Overall looks great! I'd mostly like to understand what's going on with: https://github.com/openjdk/jdk/pull/14801#discussion_r1255718844
src/hotspot/cpu/s390/downcallLinker_s390.cpp line 159:
> 157: int allocated_frame_size = 0;
> 158: assert(_abi._shadow_space_bytes == frame::z_abi_160_size, "expected space according to ABI");
> 159: allocated_frame_size = frame::z_abi_160_size;
I'm assuming that the 160 byte space is part of the C ABI, not the Java ABI, right? In that case you could just use `_abi._shadow_space_bytes` here, as this code doesn't necessarily know it's calling into C. i.e. that knowledge is captured in the Java code.
src/hotspot/cpu/s390/downcallLinker_s390.cpp line 162:
> 160: allocated_frame_size += arg_shuffle.out_arg_bytes();
> 161:
> 162: bool should_save_return_value = !_needs_return_buffer && _needs_transition;;
Since return buffer is not implemented here, I suggest adding an assert that checks that `_needs_return_buffer` is always false.
src/hotspot/cpu/s390/downcallLinker_s390.cpp line 207:
> 205: __ z_lg(callerSP, _z_abi(callers_sp), Z_SP); // preset (used to access caller frame argument slots)
> 206: __ block_comment("{ argument shuffle");
> 207: arg_shuffle.generate(_masm, as_VMStorage(callerSP), frame::z_jit_out_preserve_size, _abi._shadow_space_bytes, locs);
I'm not sure exactly what `callerSP` is doing, but it seems to be Z_SP + bias? Why can't the `in_stk_bias` parameter be used for that? (and then use `tmp` for the shuffle reg).
src/hotspot/cpu/s390/foreignGlobals_s390.cpp line 115:
> 113: switch (to_reg.type()) {
> 114: case StorageType::INTEGER:
> 115: if (to_reg.segment_mask() == REG64_MASK && from_reg.segment_mask() == REG32_MASK ) {
Since this deals with 32 bit regs as well, might want to rename the function to just `move_reg` (i.e drop the `64`)
src/hotspot/cpu/s390/foreignGlobals_s390.cpp line 186:
> 184: case StorageType::FRAME_DATA: {
> 185: switch (from_reg.stack_size()) {
> 186: case 8: __ mem2reg_opt(Z_R0_scratch, Address (callerSP, reg2offset(from_reg, in_stk_bias)), true); break;
A potential issue here is that Z_R0_scratch could be used by the target ABI, that's why the shuffle register is passed as an argument on other platforms. (It also makes it clearer in the calling code that that register is used somehow).
src/hotspot/cpu/s390/upcallLinker_s390.cpp line 141:
> 139:
> 140: // The Java call uses the JIT ABI, but we also call C.
> 141: int out_arg_area = MAX2(frame::z_jit_out_preserve_size + arg_shuffle.out_arg_bytes(), (int)frame::z_abi_160_size);
What do you mean here with "but we also call C"? Upcall stubs are always calling into Java, though the source ABI is unknown.
src/hotspot/cpu/s390/upcallLinker_s390.cpp line 172:
> 170: // |---------------------| = frame_bottom_offset = frame_size
> 171: // | (optional) |
> 172: // | ret_buf |
There's no return buffer, so this can be removed.
src/hotspot/cpu/s390/upcallLinker_s390.cpp line 232:
> 230:
> 231: // return value shuffle
> 232: if (!needs_return_buffer) {
I suggest using an assert here instead.
src/hotspot/cpu/s390/upcallLinker_s390.cpp line 243:
> 241: case T_CHAR:
> 242: case T_INT:
> 243: __ z_lgfr(Z_RET, Z_RET); // Clear garbage in high half.
Same as PPC here; this should really be done on the Java side. (See: https://github.com/openjdk/jdk/pull/12708#issuecomment-1440433079 and related discussion)
src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 78:
> 76: @CallerSensitive
> 77: public final MethodHandle downcallHandle(MemorySegment symbol, FunctionDescriptor function, Option... options) {
> 78: Reflection.ensureNativeAccess(Reflection.getCallerClass(), Linker.class, "downcallHandle");
Looks spurious? Please undo
src/java.base/share/classes/jdk/internal/foreign/abi/s390/S390Architecture.java line 115:
> 113:
> 114: private static VMStorage floatRegister(int index) {
> 115: return new VMStorage(StorageType.FLOAT, REG64_MASK, index, "v" + index);
Maybe this should be `"f"` instead of `"v"`? (given the names of the variables above)
Suggestion:
return new VMStorage(StorageType.FLOAT, REG64_MASK, index, "f" + index);
src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java line 136:
> 134: return returnLayout
> 135: .filter(GroupLayout.class::isInstance)
> 136: .filter(layout -> layout instanceof GroupLayout)
These lines both do the same, so one can be removed.
src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/TypeClass.java line 114:
> 112: return false;
> 113: }
> 114: }
I believe this loop is not needed, since above it's determined that `scalarLayouts` has only 1 element.
test/jdk/java/foreign/TestClassLoaderFindNative.java line 63:
> 61: public void testVariableSymbolLookup() {
> 62: MemorySegment segment = SymbolLookup.loaderLookup().find("c").get().reinterpret(ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN ? 1 : 4);
> 63: assertEquals(segment.get(JAVA_BYTE, ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN ? 0 : 3), 42);
Could you explain why this is needed? It looks like the lookup is returning the wrong address?
test/jdk/java/foreign/TestIllegalLink.java line 57:
> 55:
> 56: private static final boolean IS_SYSV = CABI.current() == CABI.SYS_V;
> 57: private static final boolean byteorder = ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN;
Please rename this field to something more descriptive, e.g. `IS_LE` (and capitalize).
test/jdk/java/foreign/TestLayouts.java line 46:
> 44: public class TestLayouts {
> 45:
> 46: boolean byteorder = ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN;
Same here
test/jdk/java/foreign/callarranger/platform/PlatformLayouts.java line 312:
> 310: * This class defines layout constants modelling standard primitive types supported by the S390 ABI.
> 311: */
> 312: public static final class S390 {
These are only needed if you plan on adding a CallArranger test as well.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14801#pullrequestreview-1518533244
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255680103
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255712765
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255718844
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255660955
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255723226
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255733518
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255735380
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255735815
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255743888
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255645400
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255640717
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255629673
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255624777
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255619049
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255619320
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255620308
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1255621583
More information about the core-libs-dev
mailing list