On Mon, 4 Nov 2024 18:23:23 GMT, Patricio Chilano Mateo <pchilanom...@openjdk.org> wrote:
>> 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? > >> 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828563437
