Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v6]
> 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: Preserve and restore register Z_R6 Though Z_R6 is argument register it is a saved register so preserve and restore Z_R6 register - Changes: - all: https://git.openjdk.org/jdk/pull/14801/files - new: https://git.openjdk.org/jdk/pull/14801/files/107b971f..cc2292dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14801&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14801&range=04-05 Stats: 13 lines in 2 files changed: 3 ins; 0 del; 10 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
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v6]
On Fri, 28 Jul 2023 03:59:26 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: > > Preserve and restore register Z_R6 > > Though Z_R6 is argument register it is a saved register > so preserve and restore Z_R6 register src/hotspot/cpu/s390/upcallLinker_s390.cpp line 72: > 70: // Z_SP saved/restored by prologue/epilogue > 71: if (reg == Z_SP) continue; > 72: // though Z_R6 is argument register it is a saved register Suggestion: `// although Z_R6 is used for parameter passing, it must be saved and restored by a called function.` - PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1277496026
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v6]
On Fri, 28 Jul 2023 12:36:13 GMT, Andrew Haley wrote: >> sid8606 has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Preserve and restore register Z_R6 >> >> Though Z_R6 is argument register it is a saved register >> so preserve and restore Z_R6 register > > src/hotspot/cpu/s390/upcallLinker_s390.cpp line 72: > >> 70: // Z_SP saved/restored by prologue/epilogue >> 71: if (reg == Z_SP) continue; >> 72: // though Z_R6 is argument register it is a saved register > > Suggestion: > `// although Z_R6 is used for parameter passing, it must be saved and > restored by a called function.` Thanks for the review. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1278922263
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v6]
On Fri, 28 Jul 2023 03:59:26 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:
>
> Preserve and restore register Z_R6
>
> Though Z_R6 is argument register it is a saved register
> so preserve and restore Z_R6 register
src/hotspot/cpu/s390/upcallLinker_s390.cpp line 45:
> 43: if (reg == Z_SP) continue;
> 44: // though Z_R6 is argument register it is a saved register
> 45: if (!abi.is_volatile_reg(reg) || reg == Z_R6) {
So, is the prior assumption that all argument registers are also volatile
incorrect?
If so, I think it would be better to change `ABIDescriptor::is_volatile_reg`
([1]) to only look at the `_integer_additional_volatile_registers` list (maybe
rename it too), and then list all the volatile regs in LinuxS390CallArranger
explicitly, excluding R6 ([2]). That way all the information about which regs
are volatile is in one place (LinuxS390CallArranger).
[1]:
https://github.com/openjdk/jdk/pull/14801/files#diff-7096e1975de20baa3219d616506f26ba2b4500bf5ad28c331e3d6049a32a461eR39-R42
[2]:
https://github.com/openjdk/jdk/pull/14801/files#diff-09da9016992b04bab73b6cc6aad8ca719e86c90c398af4e000583c1f8220a99bR73
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1278932461
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v6]
On Mon, 31 Jul 2023 08:08:37 GMT, Jorn Vernee wrote:
>> sid8606 has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Preserve and restore register Z_R6
>>
>> Though Z_R6 is argument register it is a saved register
>> so preserve and restore Z_R6 register
>
> src/hotspot/cpu/s390/upcallLinker_s390.cpp line 45:
>
>> 43: if (reg == Z_SP) continue;
>> 44: // though Z_R6 is argument register it is a saved register
>> 45: if (!abi.is_volatile_reg(reg) || reg == Z_R6) {
>
> So, is the prior assumption that all argument registers are also volatile
> incorrect?
>
> If so, I think it would be better to change `ABIDescriptor::is_volatile_reg`
> ([1]) to only look at the `_integer_additional_volatile_registers` list
> (maybe rename it too), and then list all the volatile regs in
> LinuxS390CallArranger explicitly, excluding R6 ([2]). That way all the
> information about which regs are volatile is in one place
> (LinuxS390CallArranger).
>
> [1]:
> https://github.com/openjdk/jdk/pull/14801/files#diff-7096e1975de20baa3219d616506f26ba2b4500bf5ad28c331e3d6049a32a461eR39-R42
> [2]:
> https://github.com/openjdk/jdk/pull/14801/files#diff-09da9016992b04bab73b6cc6aad8ca719e86c90c398af4e000583c1f8220a99bR73
Yes, s390x ABI says R6 is an argument register as we as non-volatile register.
It make sense, I'll make suggested changes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1279064461
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v6]
On Mon, 31 Jul 2023 09:51:44 GMT, sid8606 wrote:
>> src/hotspot/cpu/s390/upcallLinker_s390.cpp line 45:
>>
>>> 43: if (reg == Z_SP) continue;
>>> 44: // though Z_R6 is argument register it is a saved register
>>> 45: if (!abi.is_volatile_reg(reg) || reg == Z_R6) {
>>
>> So, is the prior assumption that all argument registers are also volatile
>> incorrect?
>>
>> If so, I think it would be better to change `ABIDescriptor::is_volatile_reg`
>> ([1]) to only look at the `_integer_additional_volatile_registers` list
>> (maybe rename it too), and then list all the volatile regs in
>> LinuxS390CallArranger explicitly, excluding R6 ([2]). That way all the
>> information about which regs are volatile is in one place
>> (LinuxS390CallArranger).
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/14801/files#diff-7096e1975de20baa3219d616506f26ba2b4500bf5ad28c331e3d6049a32a461eR39-R42
>> [2]:
>> https://github.com/openjdk/jdk/pull/14801/files#diff-09da9016992b04bab73b6cc6aad8ca719e86c90c398af4e000583c1f8220a99bR73
>
> Yes, s390x ABI says R6 is an argument register as we as non-volatile register.
> It make sense, I'll make suggested changes.
Made the changes in new commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1279354543
