[lworld] RFR: 8348961: [lworld] C2: Joining "Object:flat_in_array" with "Object:exact" does not properly work

Fri Feb 14 11:30:59 UTC 2025

On Fri, 14 Feb 2025 11:18:39 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This patch fixes the wrong joining operation of two types of the same klass with regard to flat in array. We have the following shape in the test case (how we end up with such a graph can be found as comments in the added tests):
>  
> ![image](https://github.com/user-attachments/assets/3ea680fa-d93c-4994-92b7-dd1c271c0030)
> 
> The type of `341 CheckCastPP` is `Object:exact`, so we definitely know that the type is `Object` and nothing else and thus also not flat in array (Object is not a value class). For `339 CheckCastPP`, we know that it is some kind of value class but definitely not `Object` itself - it's just chosen because we do not know more about the klass.
> 
> Joining these two should result in an empty set since they are unrelated. However, the type system is not handling it correctly and we conclude that one is a sub type of the other and we later fail with an assert.
> 
> ### Reworking flat in array Meet Operation
> I revisited the way we meet two instruction types with regard to the flat in array property. The code was a little hard to understand, so I added a summary how the flat in array property should be treated when doing a meet or join:
> 
> https://github.com/openjdk/valhalla/blob/b6b30301998e09e61b9c05282878f0596bc2f61b/src/hotspot/share/opto/type.cpp#L4604-L4630
> 
> I added a new case for joining the same klasses (see `(FiA-J-Same)`) which was missing before and resulting in an assertion failure.
> 
> ### Adding dual of `_flat_in_array`?
> I kept asking myself if we should introduce a proper lattice with a dual for flat in array:
> 
> https://github.com/openjdk/valhalla/blob/b6b30301998e09e61b9c05282878f0596bc2f61b/src/hotspot/share/opto/type.cpp#L4593-L4597
> 
> While it's probably the proper way to address this long term, I think we should investigate this separately as we would need to touch quite some code. I therefore left the idea as a comment and went with a point fix on top - keeping the way we currently handle `_flat_in_array` in meet operations without a proper dual.
> 
> ### Including Changes
> - More detailed summary how the meet/join of `flat_in_array` is done .
> - Refactored the code in `meet_instptr()` to make it more readable/clear how the `flat_in_array` property is treated.
> - For better debugging:
>   - Dumping `(flat in array)` also for `TypeInstKlassPtr` and not only for `TypeInstPtr`.
>   - Adapting "Show types" IGV filter to also show the flat in array property (see image of graph above).
> 
> ### Additionally Required Fixes
> - (i) With the type system fix above, we...

src/hotspot/share/opto/subtypenode.cpp line 55:

> 53:   // Handle inline type arrays
> 54:   if (subk->flat_in_array() && superk->not_flat_in_array_inexact()) {
> 55:     // The subtype is in flat arrays and the supertype is not in flat arrays and no subklass can be. Must be unrelated.

Case `(ii)` from PR.

src/hotspot/share/opto/subtypenode.cpp line 71:

> 69: 
> 70:   if (subk != nullptr) {
> 71:     switch (Compile::current()->static_subtype_check(superk, subk, false)) {

Should always be true and thus removed.

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

> 1026:   //    which corresponds to
> 1027:   //       !(t meet this)  meet !this ==
> 1028:   //       (!t join !this) meet !this == !this

While working on the fix I ran into the meet not symmetric assert several times and had some difficulties to figure out what type corresponds to what. I improved the comments here for better aid.

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

> 4651:     subtype_exact = below_centerline(ptr) ? (this_xk && other_xk) : (this_xk || other_xk);
> 4652:     if (above_centerline(ptr)) {
> 4653:       // Case (FiA-J-Same)

Eagerly checking this case and setting `is_empty` to make sure we don't override the result later again. Same for `(FiA-J-Sub)`.

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

> 4680: 
> 4681: 
> 4682:   if (subtype && !is_empty) {

If we already figured out that we have an empty set as result, we can just skip this code here.

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

> 4715:     res_klass = this_type->klass();
> 4716:     res_xk = this_xk;
> 4717:     res_flat_in_array = flat_in_array;

With the refactoring above, we can now just set our figured out property `flat_in_array` here directly.

src/hotspot/share/opto/type.hpp line 1805:

> 1803:   //
> 1804:   // Thus, this version checks if we know that the klass is not flat in array even if it's not exact.
> 1805:   virtual bool not_flat_in_array_inexact() const {

For Case `(ii)`.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955984673
PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955985360
PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955986907
PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955990296
PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955994621
PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955991732
PR Review Comment: https://git.openjdk.org/valhalla/pull/1367#discussion_r1955992783