On Mon, 8 Dec 2025 08:55:23 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 > > Christian Hagedorn has updated the pull request incrementally with one > additional commit since the last revision: > > update description Very nice! I only added a few minor comments. Great, that this code is finally cleaned up. src/hotspot/share/opto/type.cpp line 2846: > 2844: }; > 2845: > 2846: const char *const TypePtr::flat_in_array_msg[Uninitialized] = { Suggestion: const char* const TypePtr::flat_in_array_msg[Uninitialized] = { src/hotspot/share/opto/type.cpp line 3154: > 3152: assert(can_be_inline_type(), "only value objects can be flat in > array"); > 3153: assert(!instance_klass->is_inlinetype() || > instance_klass->as_inline_klass()->is_always_flat_in_array(), > 3154: "a value object is only marked flat in array if it proven > to be always flat in array"); Suggestion: "a value object is only marked flat in array if it's proven to be always flat in array"); src/hotspot/share/opto/type.cpp line 4571: > 4569: // centerline and or-ed above it. (N.B. Constants are always exact.) > 4570: > 4571: // Flat in Array property _flat_in_array. Nice, that this is gone now :slightly_smiling_face: src/hotspot/share/opto/type.cpp line 5404: > 5402: // below the centerline when the superclass is exact. We need > 5403: // to do the same here. > 5404: // Suggestion: src/hotspot/share/opto/type.cpp line 6272: > 6270: > 6271: uint TypeInstKlassPtr::hash(void) const { > 6272: return klass()->hash() + TypeKlassPtr::hash() + (uint)_flat_in_array; You might want to use `static_cast<uint>` here, similar to `TypeInstPtr::hash`. src/hotspot/share/opto/type.hpp line 1331: > 1329: virtual bool maybe_null() const { return meet_ptr(Null) == ptr(); } > 1330: > 1331: static FlatInArray dual_flat_in_array(FlatInArray flat_in_array) { I think this should be a non-static method so that you can access the `_flat_in_array` field directly. src/hotspot/share/opto/type.hpp line 2050: > 2048: virtual bool eq(const Type *t) const; > 2049: > 2050: Suggestion: ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1774#pullrequestreview-3550862668 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597656075 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597657401 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597669879 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597788559 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597710620 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597688028 PR Review Comment: https://git.openjdk.org/valhalla/pull/1774#discussion_r2597783100
