[lworld] RFR: 8339302: [lworld] C2: "assert(safepoints.length() == 0 || !res_type->is_inlinetypeptr()) failed: Inline type allocations should not have safepoint uses" with circular inline types [v3]

[Christian Hagedorn]

On Tue, 3 Sep 2024 12:45:44 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> The circular inline type test cases show that two asserts are too strong: They assume that we can always replace inline types with `SafePointScalarObject` nodes before doing EA with its scalar replacement phase. If we fail to do so, then there must be a missed optimization opportunity. However, this only holds for non-circular inline types. When having circular inline types, we stop the (eager) scalarization at depth 1 because we could possibly do it endlessly in a recursion. Therefore, we could have a non-scalarized `CheckCastPP` inline type oop in a safepoint which triggers the asserts.
>> 
>> I propose to relax the assert by excluding circular inline types.
>> 
>> #### Additional Required Fix
>> 
>> I also had to adjust the code which creates a `SafePointScalarObject` in the scalar replacement after EA for such a circular inline type represented by a non-`InlineTypeNode` as follows.
>> 
>> Normally, we would call `InlineTypeNode::make_scalar_in_safepoint()` to scalarize an inline type. This adds an additional input to the safepoint for the `IsInit` input to check:
>> 
>> https://github.com/openjdk/valhalla/blob/884078f06a2bf15e4be768dd09d0790b63d8e015/src/hotspot/share/opto/inlinetypenode.cpp#L280-L287
>> 
>> For example, in `testCircularSafepointUse()`, `IsInit` is false for the inline type created for `v` at L3318 and we add a top input when creating `260 SafePointScalarObject`:
>> 
>> https://github.com/openjdk/valhalla/blob/d719d86c33f87bbdb37c5a4055fbbb5f81d7937f/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestNullableInlineTypes.java#L3315-L3321
>> 
>> ![image](https://github.com/user-attachments/assets/6defb530-9a37-4701-a704-591ab391a3da)
>> 
>> The first field `v` is the `166 CheckCastPP` oop and the second field `i` has the value `188 ConI #23`. 
>> 
>> But when scalarizing the inline type field `v` later (i.e. `166 CheckCastPP`) after EA, without an `InlineTypeNode`, we miss to account for `IsInit`. We create a new `264 SafePointScalarObject` and directly append the fields after the last input (i.e. `188 ConI`) without `IsInit`:
>> 
>> ![image](https://github.com/user-attachments/assets/d5c26975-5847-4bb3-b53e-102af1471acd)
>> 
>> The first field `v` is a `21 ConP #null` and the second field `i` is the value `74 ConI #34`.
>> 
>> Later, when processing the `264 SafePointScalarObject` at code generation, we try to read the `IsInit` input which was never added:
>> 
>> https://github.com/openjdk/valhalla/blob/884078f06a2bf15e4be768dd09d0790b63d8e015/src/hotspot/shar...
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update comment

Thanks Tobias for the review and the offline suggestions!

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

PR Comment: https://git.openjdk.org/valhalla/pull/1231#issuecomment-2328020338