RFR: 8370914: C2: Reimplement Type::join [v7]

[Quan Anh Mai]

On Wed, 7 Jan 2026 15:03:44 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains nine commits:
>> 
>>  - copyright year
>>  - Merge branch 'master' into typejoin
>>  - sort order
>>  - Merge branch 'master' into typejoin
>>  - Merge branch 'master' into typejoin
>>  - Move dual to ASSERT only
>>  - Keep old version for verification
>>  - whitespace
>>  - Reimplement Type::join
>
> src/hotspot/share/opto/rangeinference.cpp line 700:
> 
>> 698:     return int_type_union(t1, t2);
>> 699:   } else {
>> 700:     return CT::make_or_top(TypeIntPrototype<S, U>{{MAX2(t1->_lo, t2->_lo), MIN2(t1->_hi, t2->_hi)},
> 
> Nothing dramatic, but why is this branch implemented directly, why the other branch is calling another function `int_type_union`. This function seems to be used only here and in testing. If I understand correctly, this branch is meant to go away when the `_is_dual` is removed, but it is (almost) the same as in `int_type_xjoin` (which makes sense).
> 
> I'm not very decided what would be the best, but I'm slightly annoyed they look different. Should we have a `int_type_intersection`? But I don't see what it would do that `int_type_xjoin` doesn't do. Or simply, why do we need `int_type_union`? Once this else-branch removed, wouldn't `int_type_xmeet` simply be a call to `int_type_union`, and so maybe we could avoid this extra-step?
> 
> I'm fine if the answer is "it's part of the future clean up work", but then, I think we could have a tracking ticket and a TODO comment.

The asymmetry comes from the fact that a union of 2 non-empty sets is always non-empty, while the intersection may not, and there is no construct for `TypeIntMirror-or-empty` yet. I have at least refactored the `TypeIntPrototype` computation to a common function.

> src/hotspot/share/opto/rangeinference.cpp line 710:
> 
>> 708: 
>> 709: template <class CT>
>> 710: const Type* TypeIntHelper::int_type_xjoin(const CT* t1, const CT* t2) {
> 
> Unlike `int_type_union`, I couldn't find test for `int_type_xjoin`. have I missed something? Is that expected? I agree it's not the the most scary function, but... why not! And it would at least be there for skeleton when adding other abstract domains that might be less obvious.

It is a pretty trivial function, and similar to other `meet` and `join`, it is tested against fundamental set operation laws. I don't think there is a test for `int_type_union`, either. It appears in the test file because we want to meet `TypeIntMirror`s there.

> src/hotspot/share/opto/type.cpp line 1029:
> 
>> 1027:     tty->print("t1 meets t2 = "); mt1->dump(); tty->cr();
>> 1028:     tty->print("t2 meets t1 = "); mt2->dump(); tty->cr();
>> 1029:     fatal("meet not commutative");
> 
> I see it was like that before, but I think it's discouraged to have many tty->print to avoid interleaved output of concurrent prints. The preferred solution is to use a stringstream rather than a lock. Also, do we need a flush before the fatal? I've seen some (other) prints that are cut before the end on assert failures.
> 
> Same under.

That's a good idea, I have replaced the series of `tty->print` in this function with a `stringStream`

> src/hotspot/share/opto/type.cpp line 2401:
> 
>> 2399: 
>> 2400: //------------------------------meet-------------------------------------------
>> 2401: // Compute the MEET of two types.  It returns a new Type object.
> 
> Should we clean up these comments? I think we can have it on the base method, but maybe not that useful on each override? Also, it's not a great comment: it doesn't bring much more compared to the signature, it speaks abotu two types, but one of them is `this` and so implicit which is somewhat confusing (it would make more sense on `const Type* Type::xmeet(const Type* t1, const Type* t2)`). The comments says it returns a new Type object, which is ambiguous: these methods can return `this` or `t`, which are not new objects, for most definitions I'd give to this. I think it means that it doesn't mutate `this` or `t`, but actually return the result, but that is clear from the `const`s.
> 
> Some of these comments have a header line of dashes where it's written `xmeet` and some (like this one) only `meet`.
> 
> Overall, I think these comments bring nothing, or possibly confusion, and it would be a good opportunity to get rid of them.

Done, I have removed them

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28051#discussion_r2669527966
PR Review Comment: https://git.openjdk.org/jdk/pull/28051#discussion_r2669521513
PR Review Comment: https://git.openjdk.org/jdk/pull/28051#discussion_r2669511484
PR Review Comment: https://git.openjdk.org/jdk/pull/28051#discussion_r2669509065