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

[Christian Hagedorn]

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...

src/hotspot/share/opto/graphKit.cpp line 3451:

> 3449:   const TypeKlassPtr* klass_ptr_type = _gvn.type(superklass)->is_klassptr();
> 3450:   const TypeKlassPtr* improved_klass_ptr_type = klass_ptr_type->try_improve();
> 3451:   const TypeOopPtr* toop = improved_klass_ptr_type->cast_to_exactness(false)->as_instance_type();

Changes in this file taken from JDK-8328480.

src/hotspot/share/opto/type.cpp line 997:

> 995: 
> 996:   // Verify that:
> 997:   //      this meet t == t meet this

Found these comments useful while verifying the type system changes.

src/hotspot/share/opto/type.cpp line 4536:

> 4534:   const bool other_flat_in_array = other_type->flat_in_array();
> 4535:   const bool this_not_flat_in_array = this_type->not_flat_in_array();
> 4536:   const bool other_not_flat_in_array = other_type->not_flat_in_array();

`this_flat_in_array` and `flat_array` are both used to hold intermediate and final results about whether the type is going to be flat-in-array or not. This was somewhat hard to follow what was going on.

Changed this into: `this_flat_in_array` for the flat-in-array state of `this_type` and `flat_in_array` to hold the intermediate and eventually the final result assigned to `res_flat_in_array`.

src/hotspot/share/opto/type.cpp line 4582:

> 4580:     subtype_exact = below_centerline(ptr) ? (this_xk && other_xk) : (this_xk || other_xk);
> 4581:     flat_array = below_centerline(ptr) ? (this_flat_in_array && other_flat_in_array) : (this_flat_in_array || other_flat_in_array);
> 4582:   } else if (!other_xk && this_type->is_meet_subtype_of(other_type) && (!other_flat_in_array || this_flat_in_array)) {

Extracted `is_meet_subtype_of()` + flat-in-array checks to separate methods to simplify the condition.

src/hotspot/share/opto/type.cpp line 4604:

> 4602:       other_type = this_type; // this is down; keep down man
> 4603:       other_xk = this_xk;
> 4604:       other_flat_in_array = this_flat_in_array;

Assignments to `other*` are not used anymore later and are thus removed.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1572215555
PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1572215040
PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1572228027
PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1572220233
PR Review Comment: https://git.openjdk.org/valhalla/pull/1079#discussion_r1572216420