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

[Christian Hagedorn]

On Tue, 3 Sep 2024 09:19:19 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/share/opto/output.cpp#L809-L816
> 
> The first input is `21 ConP #...

This pull request has now been integrated.

Changeset: 3084f84b
Author:    Christian Hagedorn <chagedorn at openjdk.org>
URL:       https://git.openjdk.org/valhalla/commit/3084f84b0e77c9c7693842b3044939a5f535018b
Stats:     74 lines in 2 files changed: 71 ins; 1 del; 2 mod

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

Reviewed-by: thartmann

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

PR: https://git.openjdk.org/valhalla/pull/1231