[lworld] RFR: 8351006: [lworld] Refactor LoadNode::Identity of an InlineType into can_see_stored_value [v2]

[Christian Hagedorn]

On Wed, 5 Mar 2025 12:35:38 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   assert non-larval
>
> Nice refactoring!
> 
>> LoadUS from a short is not a mismatched access and hence can return a wrong value
> 
> Do you have a test case where we get a wrong execution? If so, it would be great to add it as well :-)

> @chhagedorn Thanks for your reviews
> 
> > Do you have a test case where we get a wrong execution? If so, it would be great to add it as well :-)
> 
> Unfortunately, I cannot, this pops up when I'm working on another change.

Okay, no problem.

> > Is there a specific reason that you made this `static` and not a member function of `MemNode` (then you can use `this` instead of `load`)?
> 
> I think using `static` offers a more local scope, less visibility, and reduce convoluting the class declaration with helper methods.

I understand. I'm not quite sure about the visibility, though. It's now local to `memnode.cpp` which is huge. Moreover, it's not immediately clear to what class this method belongs to or from where it should be called from. Moving it to `MemNode` makes the intention clearer I think. But of course, it's just a personal opinion :-)

-------------

PR Comment: https://git.openjdk.org/valhalla/pull/1387#issuecomment-2704537011