On Fri, 7 Jul 2023 11:56:28 GMT, Jorn Vernee <jver...@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
