[lworld] RFR: 8354068: [lworld] Fold strict final fields more aggressively [v2]

[Quan Anh Mai]

On Wed, 23 Apr 2025 18:35:55 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - fix wrong results
>>  - fix invalid node class: Unlock
>
> src/hotspot/share/opto/memnode.cpp line 237:
> 
>> 235: // current compilation unit, or is the first parameter when we are in a constructor
>> 236: static bool call_can_modify_local_object(ciField* field, CallNode* call, Node* base_local) {
>> 237:   // The fields can only be modified in this method or in a constructor
> 
> Comments and naming of this method are a bit confusing. Isn't this basically checking if `base_local` is the receiver of a constructor call `call`?

Yes the comment should refer to `base_local` not `base`. I have also added a section clarifying that this method is equivalent to asking whether `base_local` is the receiver of a constructor call `call` and the holder of `call` is a subclass of the holder of `field`.

> src/hotspot/share/opto/memnode.cpp line 248:
> 
>> 246: 
>> 247:   Node* parm = call->in(TypeFunc::Parms);
>> 248:   if (parm->uncast() == base_local || (parm->is_InlineType() && parm->as_InlineType()->get_oop()->uncast() == base_local)) {
> 
> There's some code duplication with the checks in `optimize_strict_final_load_memory_from_local_object`, could that be factored out? Especially since the comments there are helpful.

Done so in the new commit

> src/hotspot/share/opto/memnode.cpp line 288:
> 
>> 286:     if (tmp != nullptr) {
>> 287:       result = tmp;
>> 288:     }
> 
> Suggestion:
> 
>     result = optimize_strict_final_load_memory(phase, field, adr, base_local);
>     if (result == nullptr) {
>       result = mchain;
>     }

I find it easier to say that this method tries to find a memory node and if it succeeds we set `result` to the found node.

> src/hotspot/share/opto/memnode.cpp line 330:
> 
>> 328:             // Allocation of another type, must be another object
>> 329:             result = proj_in->in(TypeFunc::Memory);
>> 330:           } else if (base_local != nullptr && (base_local->is_Parm() || base_local->in(0) != alloc)) {
> 
> Should this be changed to `base_local->as_Proj()->in(0)` to assert that `base_local` is a projection?

I have refactored `base_local` to be a `ProjNode*` to make this more explicit.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2057151605
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2057150487
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2057152806
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2057144999