RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v2]

[sid8606]

On Fri, 7 Jul 2023 11:56:28 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> sid8606 has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address Amit's review comments
>
> 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.

Added assert.

> 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`)

Renamed.

> 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

Fixed

> 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);

Fixed

> 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.

Fixed

> 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.

Removed, Thank you

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257228786
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257228670
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257228415
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257228265
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257228164
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257228044