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

[Tobias Hartmann]

On Wed, 9 Apr 2025 17:46:35 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> When we do incremental inlining, we need to be more aggressive to ensure that value objects are not unexpectedly materialized. This is done by trying to fold loads from strict final fields in an aggressive manner.
>> 
>> Please take a look and leave your reviews, thanks a lot.
>
> 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

Testing all passed. I had a first look at the changes and left a few comments. Will have another look tomorrow.

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`?

src/hotspot/share/opto/memnode.cpp line 238:

> 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
> 238:   if (!call->is_CallJava() || call->req() <= TypeFunc::Parms) {

Isn't it guaranteed for an object constructor to always have `call->req() > TypeFunc::Parms`? So maybe this should be an assert.

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.

src/hotspot/share/opto/memnode.cpp line 257:

> 255:   assert(t_oop != nullptr, "sanity");
> 256:   bool is_instance = t_oop->is_known_instance_field();
> 257: 

What about the `is_boxed_value_load`? If Valhalla / preview is disabled, these wouldn't be covered by `is_strict_final_load` right?

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;
    }

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?

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

PR Review: https://git.openjdk.org/valhalla/pull/1424#pullrequestreview-2787272535
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2056679081
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2056278539
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2056714221
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2056031664
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2056231927
PR Review Comment: https://git.openjdk.org/valhalla/pull/1424#discussion_r2056228558