[lworld] RFR: 8321734: [lworld] C2: flat_in_array type is not handled correctly leading to crashes

[Tobias Hartmann]

On Fri, 19 Apr 2024 11:13:50 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> There are multiple issues around types being `flat_in_array`:
> 
> #### 1. `SubTypeCheckNode` is not folded while `CheckCastPP` is correctly replaced by top due to an impossible type:
> Test: `testSubTypeCheckNotFoldedParsingAbstractClass():` 
> 
> We have the following:
> - `PUnique` is the unique concrete value sub class of the abstract value `AUnique` class
> - `Object[] arr` -> elements could be an inline type that is flat
> 
> We apply loop unswitching such that we have a loop where the access `arr[i]` is with a flat inline type (i.e. marked as flat-in-array) and one version without. In the flat-in-array loop, we have the following sub type check for the `checkcast` for `(AUnique)arr[i]`:
> 
> AUnique [maybe-flat-in-array] <: Object [flat-in-array]
> 
> Since we do not know if `AUnique` is flat-in-array, we cannot fold this check. However, the corresponding `CheckCastPP` for this sub type check uses the unique concrete sub class `PUnique` which is known to be *not*-flat-in-array with `FlatArrayElementMaxSize=4`. Since a not-flat-in-array type cannot be a sub type of a flat-in-array type (determined during class loading), the `CheckCastPP` is replaced by top. But the `SubTypeCheck` is not folded. 
> 
> This was found in Valhalla and fixed in mainline with JDK-8328480. I've added the Valhalla specific test and merged the fix from mainline into this patch.
> 
> #### 2. Wrong `not_flat_in_array` default value
> Test: `testSubTypeCheck()` and `testCmpP()`:
> Arrays are always normal objects (i.e. not inline types). They are therefore never flat-in-array and always not-flat-in-array. While they are correctly never marked as flat-in-array, they are currently not marked as *not*-flat-in-array and thus have the implicit state *maybe*-flat-in-array:
> https://github.com/openjdk/valhalla/blob/e01ec832189453cc302c7ca8915e69bb63a3d4b1/src/hotspot/share/opto/type.hpp#L1096
> 
> This becomes a problem for `SubTypeCheck` and `CmpP` nodes which cannot determine that a flat-in-array klass and an array klass are unrelated. But the type system itself correctly treat arrays as *not*-flat-in-array when doing a meet operation. As a result, data is folded while control is not.
> 
> This is fixed by changing the flat-in-array tri-state: As a default, a type is always not-flat-in-array. Inline types then override this property accordingly.
> 
> #### 3. Type system wrongly treats "maybe-be-flat-in-array <: flat-in-array" as top
> Test: `testUnswitchingAbstractClass()`
>  "maybe-be-flat-in-array <: flat-in-array" should not be treate...

Thanks for the thorough investigation, Christian. Great job! The fixes look good to me.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFlatInArraysFolding.java line 30:

> 28:  * @summary Test that CmpPNode::sub and SubTypeCheckNode::sub correctly identify unrelated classes based on the flat
> 29:  *          in array property of the types. Additionally check that the type system properly handles the case of a
> 30:  *         super class being flat in array while the sub klass could be flat in array.

Suggestion:

 *          super class being flat in array while the sub klass could be flat in array.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFlatInArraysFolding.java line 43:

> 41:  * @summary Test that CmpPNode::sub and SubTypeCheckNode::sub correctly identify unrelated classes based on the flat
> 42:  *          in array property of the types. Additionally check that the type system properly handles the case of a
> 43:  *         super class being flat in array while the sub klass could be flat in array.

Suggestion:

 *          super class being flat in array while the sub klass could be flat in array.

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1079#pullrequestreview-2019615504
PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1577736697
PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1577736833