On Mon, 8 Dec 2025 08:13:34 GMT, Christian Hagedorn <[email protected]> wrote:
> This patch turns the boolean flag `_flat_in_array` into a new `FlatInArray` > enum to properly define a (Boolean) lattice. This mainly allows us to cleanly > represent "maybe flat in array" and "not flat in array". The dedicated top > element is the dual of "maybe flat in array". > > To simplify the review and to better understand what the changes are, I added > code comments directly in the PR. > > Here is a high-level overview of the changes: > - `FlatInArray` enum to define a lattice for `flat_in_array`: > https://github.com/openjdk/valhalla/blob/6b1930c7b9a359223a998d0086a1326a9d7905e7/src/hotspot/share/opto/type.hpp#L1182-L1199 > - Updated `meet` operations accordingly. Needed to be careful when doing the > meet above the centerline. > - Added single `compute_flat_in_array()` method to properly compute the > `flat_in_array` property from the information we have when we need to set > flat in array newly (this was no properly done before). > - In `flatten_alias_type()`, we flatten to "maybe flat in array". > > #### Testing: > - t1-4 + valhalla-comp-stress > > Thanks for your feedback, > Christian src/hotspot/share/ci/ciInlineKlass.cpp line 41: > 39: > 40: // Are arrays containing an instance of this value class always flat? > 41: bool ciInlineKlass::is_always_flat_in_array() const { Used for `compute_flat_in_array()`. src/hotspot/share/ci/ciInstanceKlass.cpp line 708: > 706: if (!is_exact) { > 707: // Not exact, check if this is a valid super for an inline klass > 708: GUARDED_VM_ENTRY( Is now called from more paths over `compute_flat_in_array()`. As a result, hit assert during testing because thread was already in VM state - changed to `GUARDED_VM_ENTRY`. src/hotspot/share/opto/subtypenode.cpp line 53: > 51: bool unrelated_classes = false; > 52: // Handle inline type arrays > 53: if (subk->flat_in_array() && superk->not_flat_in_array_inexact()) { The following logic was hidden in `not_flat_in_array_inexact()` and not completely accurate. Improved now. src/hotspot/share/opto/type.cpp line 1068: > 1066: // 2) > 1067: tty->print("mt_dual meet this_dual= "); t2this ->dump(); > tty->cr(); > 1068: tty->cr(); Since I hit the assert a few times while working on this patch, I added some more printing code for easier debugging. I guess it could also be useful in the future. src/hotspot/share/opto/type.cpp line 3211: > 3209: if (!Verbose) { > 3210: break; > 3211: } Can be discussed which state we want to dump by default. It might be too verbose to always dump the exact state. src/hotspot/share/opto/type.cpp line 4169: > 4167: "cannot have constants with non-loaded klass"); > 4168: assert(!klass()->maybe_flat_in_array() || flat_in_array, "Should be > flat in array"); > 4169: assert(!flat_in_array || can_be_inline_type(), "Only inline types can > be flat in array"); Is now checked with `compute_flat_in_array()`. src/hotspot/share/opto/type.cpp line 4268: > 4266: if (flat_in_array == Uninitialized) { > 4267: flat_in_array = compute_flat_in_array(ik, xk); > 4268: } We use the special `Uninitialized` state to mark that we should compute the flat in array property from the information we got. `Uninitialized` has no other use. src/hotspot/share/opto/type.cpp line 4517: > 4515: > 4516: template<class T> TypePtr::MeetResult TypePtr::meet_instptr(PTR& ptr, > const TypeInterfaces*& interfaces, const T* this_type, const T* other_type, > 4517: ciKlass*& > res_klass, bool& res_xk, bool& res_flat_in_array) { This method now becomes much simpler without handling the meet of flat in array. src/hotspot/share/opto/type.cpp line 4533: > 4531: bool res_xk = false; > 4532: const Type* res; > 4533: MeetResult kind = meet_instptr(ptr, interfaces, this, tinst, > res_klass, res_xk); I removed the meet of flat in array from `meet_instptr()` into a separate `meet_flat_in_array()` to simplify the logic. src/hotspot/share/opto/type.cpp line 4744: > 4742: const Type* TypeInstPtr::xdual() const { > 4743: return new TypeInstPtr(dual_ptr(), klass(), _interfaces, > klass_is_exact(), const_oop(), dual_offset(), > 4744: dual_flat_in_array(_flat_in_array), > dual_instance_id(), dual_speculative(), dual_inline_depth()); Take proper dual for `_flat_in_array`. src/hotspot/share/opto/type.cpp line 4836: > 4834: } > 4835: return TypeOopPtr::empty(); > 4836: } Since we have now a proper `TopFlat` element in the lattice, we can introduce an `empty()` method to check if the type is above the centerline and thus empty. src/hotspot/share/opto/type.cpp line 4878: > 4876: } > 4877: > 4878: const TypeInstPtr *TypeInstPtr::cast_to_maybe_flat_in_array() const { Used by `flatten_alias_type()`. src/hotspot/share/opto/type.cpp line 5395: > 5393: case Constant: > 5394: case NotNull: > 5395: case BotPTR: { // Fall down to object klass Required to add braces, otherwise, this does not build. src/hotspot/share/opto/type.cpp line 6225: > 6223: if (flat_in_array() && !klass()->is_inlinetype()) { > 6224: st->print(" (flat in array)"); > 6225: } Now done in `TypeInstKlassPtr::dump2()`. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597494142 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597503433 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597511101 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597514427 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597519218 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597521681 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597525113 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597532200 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597529087 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597535200 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597539928 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597541428 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597559003 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597560905
