On Tue, 5 Nov 2024 00:18:17 GMT, Fei Yang <fy...@openjdk.org> wrote: >>> 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 think the previous lines are okay because we are constructing the fp, >> whereas in here we want to read the old fp stored in this frame. > >> 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828566395
