[lworld] RFR: 8335256: [lworld] compiler/valhalla/inlinetypes/TestValueConstruction.java fails intermittently [v3]

[Jatin Bhateja]

On Tue, 11 Feb 2025 15:51:04 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> The assert fires because in this place:
>> 
>>     if (is_osr_parse()) {
>>       Node* osr_buf = entry_map->in(TypeFunc::Parms+0);
>>       entry_map->set_req(TypeFunc::Parms+0, top());
>>       set_map(entry_map);
>>       load_interpreter_state(osr_buf);
>>     } else {
>>       set_map(entry_map);
>>       do_method_entry();
>>     }
>> 
>> `load_interpreter_state` assumes that all incoming inline types are non-larval, while in the following place:
>> 
>>     if (t->is_inlinetypeptr()) {
>>       // Create InlineTypeNode from the oop and replace the parameter
>>       bool is_larval = (i == 0) && method()->is_object_constructor() && !method()->holder()->is_java_lang_Object();
>>       Node* vt = InlineTypeNode::make_from_oop(this, parm, t->inline_klass(), !t->maybe_null(), is_larval);
>>       replace_in_map(parm, vt);
>>     }
>> 
>> `is_larval == true` because we are osr-compiling the constructor of a value class. As a result, we pass a non-larval object into `InlineTypeNode::make_from_oop` and want to make it larval, triggering the assert.
>> 
>> Thinking more deeply into this, I think there is no way we can know the larval-ness of an incoming object in an osr-compilation unless `ciTypeFlow` does so. For example, consider this bytecode sequence:
>> 
>>     ValueObject x = new ValueObject;
>>     for {
>>     }
>>     x.<init>();
>> 
>> Then, when we osr-compile the method from the loop, `x` will be a larval object and it can be at any place on the JVM stack or in the local slots.
>> 
>> As a result, I propose optimistically assuming all osr parameters be non-larval, then we can change them to larval when encountering a constructor invocation or a field store instruction. The verifier should ensure that we cannot do anything else on a larval object as well as do these actions on a non-larval object, so we should recognize the correct state of the value object if any operation is performed on them. Otherwise, they should be dead and not apear here.
>> 
>> Please take a look and give your advices, thanks a lot.
>
> Quan Anh Mai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'lworld' into osrinlinetype
>  - typo
>  - Fix osr inline type parameter

src/hotspot/share/opto/doCall.cpp line 588:

> 586:     // optimistically assume all parameters are non-larval. Then, a parameter is changed to larval
> 587:     // if we encounter a store into one of its fields, or if we encounter a constructor invocation
> 588:     // with it being the first argument

The implicit larval transition has a limited life span, it begins from the new allocation and ends just before exiting constructor.

[https://github.com/openjdk/valhalla/blob/d94c58952d2f9a0193af4cd9df66868fd0a6e3db/src/hotspot/share/opto/doCall.cpp#L830](https://github.com/openjdk/valhalla/blob/d94c58952d2f9a0193af4cd9df66868fd0a6e3db/src/hotspot/share/opto/doCall.cpp#L830
)

The only possibility to create a larval value that gets passed on to OSR buffer is by explicitly marking it as larval using Unsafe.makePrivateBuffer API before the loop.

May I request you to kindly add a new or identify an existing test case for this scenario

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1358#discussion_r1954316155