On Tue, 5 Nov 2024 00:23:37 GMT, Fei Yang <fy...@openjdk.org> wrote: >>> As the same code on aarch64 and x86-64 uses `frame::sender_sp_offset` I >>> suggested to change the literal 2 into `frame::sender_sp_offset` in order >>> to increase the readability, but I forgot that `frame::sender_sp_offset` is >>> 0 on riscv64. However I do think it's a problem that several places >>> throughout the code base uses a literal 2 when it should really be >>> `frame::sender_sp_offset`. This type of code is very fiddly and I think we >>> should do what we can to increase the readability, so maybe we need another >>> `frame::XYZ` constant that is 2 for this case. >> >> Yeah, I was also considering this issue when we were porting loom. I guess >> maybe `frame::metadata_words` which equals 2. Since this is not the only >> place, I would suggest we do a separate cleanup PR. >> >>> Also, does this mean that the changes from 2 to `frame::sender_sp_offset` >>> in all of the lines (267, 271 and 273) should be reverted? >> >> I agree with @pchilano in that we are fine with these places. > >> Sorry, I also thought it matched the aarch64 one without checking. >> @RealFYang should I change it for `hf.sp() + frame::link_offset` or just >> leave it as it was? > > Or maybe `hf.sp() - frame::metadata_words`. But since we have several other > occurrences, I would suggest we leave it as it was and go with a separate PR > for the cleanup.
Reverted. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828615499
