[lworld+vector] RFR: 8320815: [lworld+vector] Fix vector API jtreg crashes with "-XX:InlineFieldMaxFlatSize=0"

[Jatin Bhateja]

On Mon, 4 Dec 2023 08:50:45 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

> Current Vector API jtreg crashes with `-XX:InlineFieldMaxFlatSize=0` with several kinds of issues. This option makes all the value object fields can not be flattened.
> 
> This patch fixes them with following main changes:
> 
>  1) Remove the fields count check for `Vector/VectorPayload` objects when scalaring the value objects across safepoints. The check on vector objects fails when its payload field is not flattened (see [1]). The check is not right and can be removed.
> 
>  2) Insert two additional inputs of `InlineTypeNode` to safepoint when creating the `SafePointScalarObjectNode` for a value object. This is missing when compiler handles an allocation merge with inline type. It causes the assertion [2] fails.
> 
>  3) Forbit optimizing a `LoadN` to `EncodeP (InlineType)`, if the `InlineType` is the payload field of `VectorBox`. This fixes the
> assertion failure [3].
> 
> When loading from an `InlineType`, C2 compiler tries to look for its field value with the matching offset. If the matched field is a
> flattened value object, its field with the matching offset is searched recursively. Otherwise, the field itself is used. After the field
> value is found, the load can be saved, and the field value is used as the replacement. If it is an object loading (i.e. `LoadN`), the
> matched field value is expected to be a valid oop.
> 
> The issue happens when the address base is `VectorBox` and the matched field is a non-flattened `InlineType`. Since `VectorBox` is expanded lately, its memory and the memory for the field are not allocated at this moment. Optimizing `LoadN` to `EncodeP` with the un-allocated `InlineType` makes it possible to the miss the buffer.
> 
>  4) Explicitly check whether the payload field is flattened when expanding `VectorBox`. Allocate the buffer and store the vector value to the memory if the payload is not flattened.
> 
> The similar operations are handled in `InlineTypeNode::store()` before. But the graph is more complex and not right for some vector API cases. We hit assertion [4].
> 
> Tests: All tests pass with `-XX:InlineFieldMaxFlatSize=0` now. And no new regressions are found.
> 
> [1] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/inlinetypenode.cpp#L318
> [2] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/output.cpp#L812
> [3] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/compile.cpp#L2050
> [4] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/in...

src/hotspot/share/opto/inlinetypenode.cpp line 319:

> 317:   } else if (is_vector_payload(vk)) {
> 318:      assert(field_value(0)->as_InlineType()->field_count() == nfields, "");
> 319:   }

Its not very clear, why will these assertions offend because C2 IR is in synchronism with ci Model. Also given that C2 always creates a InlineTypeNode for a value object. Even for a non-flattened field compile injects a re-materializing InlineTypeNode from the loaded value oop.

src/hotspot/share/opto/macro.cpp line 860:

> 858:     sfpt->add_req(_igvn.intcon(1));
> 859:     sfpt->add_req(_igvn.intcon(alloc->_larval ? 1 : 0));
> 860:   }

We have a separate pass ["process_inline_types"](https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/opto/compile.cpp#L1977), which walks over _inline_type_nodes and scalarizes them at safe points.  I think we can even skip over doing any special handling for InlineTypeNodes in regular escape analysis. WDYT ?

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/958#discussion_r1420127683
PR Review Comment: https://git.openjdk.org/valhalla/pull/958#discussion_r1420149389