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

[sid8606]

On Sun, 6 Aug 2023 09:32:08 GMT, Andrey Turbanov  wrote:

>> sid8606 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix typo
>>   
>>   Signed-off-by: Sidraya 
>
> src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java
>  line 112:
> 
>> 110: }
>> 111: 
>> 112: public static  MethodHandle arrangeDowncall(MethodType mt, 
>> FunctionDescriptor cDesc, LinkerOptions options) {
> 
> Suggestion:
> 
> public static MethodHandle arrangeDowncall(MethodType mt, 
> FunctionDescriptor cDesc, LinkerOptions options) {

Thank you @turbanoff for the review comment. I have Fixed this in new commit.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613222

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

[sid8606]

On Mon, 7 Aug 2023 13:37:57 GMT, Martin Doerr  wrote:

>> sid8606 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix typo
>>   
>>   Signed-off-by: Sidraya 
>
> src/hotspot/cpu/s390/foreignGlobals_s390.cpp line 154:
> 
>> 152:   } else {
>> 153: assert(to_reg.stack_size() == 4, "size should match");
>> 154: // s390 needs 4 Byte offset
> 
> Seems like this comment should get removed.

Fixed

> src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java
>  line 221:
> 
>> 219: Class type = 
>> SharedUtils.primitiveCarrierForSize(layout.byteSize(), false);
>> 220: bindings.bufferLoad(0, type)
>> 221: .vmStore(storage, type);
> 
> Maybe improve indentation?

Thank you @TheRealMDoerr. Fixed in new commit.

> src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/TypeClass.java
>  line 44:
> 
>> 42: FLOAT;
>> 43: 
>> 44: private static final int MAX_AGGREGATE_REGS_SIZE = 1;
> 
> Unused.

Fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613915
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613616
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613721

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

[Martin Doerr]

On Tue, 1 Aug 2023 06:29:28 GMT, sid8606  wrote:

>> Implementation of "Foreign Function & Memory API" for s390x (Big Endian).
>
> sid8606 has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Fix typo
>   
>   Signed-off-by: Sidraya 

src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java
 line 221:

> 219: Class type = 
> SharedUtils.primitiveCarrierForSize(layout.byteSize(), false);
> 220: bindings.bufferLoad(0, type)
> 221: .vmStore(storage, type);

Maybe improve indentation?

src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/TypeClass.java 
line 44:

> 42: FLOAT;
> 43: 
> 44: private static final int MAX_AGGREGATE_REGS_SIZE = 1;

Unused.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285885012
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285886685

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

[Martin Doerr]

On Tue, 1 Aug 2023 06:29:28 GMT, sid8606  wrote:

>> Implementation of "Foreign Function & Memory API" for s390x (Big Endian).
>
> sid8606 has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Fix typo
>   
>   Signed-off-by: Sidraya 

src/hotspot/cpu/s390/foreignGlobals_s390.cpp line 154:

> 152:   } else {
> 153: assert(to_reg.stack_size() == 4, "size should match");
> 154: // s390 needs 4 Byte offset

Seems like this comment should get removed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285880721

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

[Andrey Turbanov]

On Tue, 1 Aug 2023 06:29:28 GMT, sid8606  wrote:

>> Implementation of "Foreign Function & Memory API" for s390x (Big Endian).
>
> sid8606 has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Fix typo
>   
>   Signed-off-by: Sidraya 

src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java
 line 112:

> 110: }
> 111: 
> 112: public static  MethodHandle arrangeDowncall(MethodType mt, 
> FunctionDescriptor cDesc, LinkerOptions options) {

Suggestion:

public static MethodHandle arrangeDowncall(MethodType mt, 
FunctionDescriptor cDesc, LinkerOptions options) {

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285183843

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

[sid8606]

> Implementation of "Foreign Function & Memory API" for s390x (Big Endian).

sid8606 has updated the pull request incrementally with one additional commit 
since the last revision:

  Fix typo
  
  Signed-off-by: Sidraya 

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14801/files
  - new: https://git.openjdk.org/jdk/pull/14801/files/b81d5bb7..924b45bc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14801=09
 - incr: https://webrevs.openjdk.org/?repo=jdk=14801=08-09

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14801.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14801/head:pull/14801

PR: https://git.openjdk.org/jdk/pull/14801