On Fri, 7 Jul 2023 12:06:39 GMT, Jorn Vernee <[email protected]> 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/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).
Now using Z_R11 frame pointer and passing shuffle register to use as temp.
> 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.
Fixed
> 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.
Added an assert, thanks
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257230962
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257231066
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1257231195
