RFR: 8315066: Add unsigned bounds and known bits to TypeInt/Long [v32]

Mon Jan 27 14:49:03 UTC 2025

On Wed, 22 Jan 2025 15:43:03 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> This patch adds unsigned bounds and known bits constraints to TypeInt and TypeLong. This opens more transformation opportunities in an elegant manner as well as helps avoid some ad-hoc rules in Hotspot.
>> 
>> In general, a `TypeInt/Long` represents a set of values `x` that satisfies: `x s>= lo && x s<= hi && x u>= ulo && x u<= uhi && (x & zeros) == 0 && (x & ones) == ones`. These constraints are not independent, e.g. an int that lies in [0, 3] in signed domain must also lie in [0, 3] in unsigned domain and have all bits but the last 2 being unset. As a result, we must canonicalize the constraints (tighten the constraints so that they are optimal) before constructing a `TypeInt/Long` instance.
>> 
>> This is extracted from #15440 , node value transformations are left for later PRs. I have also added unit tests to verify the soundness of constraint normalization.
>> 
>> Please kindly review, thanks a lot.
>> 
>> Testing
>> 
>> - [x] GHA
>> - [x] Linux x64, tier 1-4
>
> 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 44 commits:
> 
>  - remove precompiled.hpp
>  - Merge branch 'master' into unsignedbounds
>  - copyright
>  - Merge branch 'master' into unsignedbounds
>  - Merge branch 'master' into unsignedbounds
>  - move try_cast to Type
>  - Merge branch 'master' into unsignedbounds
>  - build failure
>  - build failures
>  - whitespace
>  - ... and 34 more: https://git.openjdk.org/jdk/compare/893d00ac...c4d46c87

A few more for today. More tomorrow or later.

src/hotspot/share/opto/rangeinference.hpp line 145:

> 143:     // srange intersects with urange in 2 intervals [srange._lo, urange._hi]
> 144:     // and [urange._lo, srange._hi]
> 145:     return (urange._hi - U(srange._lo)) + (U(srange._hi) - urange._lo) + 1;

Ah, you are adding +1 here, but not above. I think that is because above it is one range, but here 2, right? Would be nice if you made a comment about that too.

src/hotspot/share/opto/rangeinference.hpp line 149:

> 147: 
> 148:   template <class CT, class S, class U>
> 149:   static const Type* int_type_xmeet(const CT* i1, const Type* t2, const Type* (*make)(const TypeIntPrototype<S, U>&, int, bool), bool dual);

I would create a typedef for the function pointer. Otherwise my brain only sees a salad of  asterisks, brackets, keywords etc 😅

src/hotspot/share/opto/rangeinference.hpp line 162:

> 160:     return super->_lo <= sub->_lo && super->_hi >= sub->_hi &&
> 161:            super->_ulo <= sub->_ulo && super->_uhi >= sub->_uhi &&
> 162:            (super->_bits._zeros &~ sub->_bits._zeros) == 0 && (super->_bits._ones &~ sub->_bits._ones) == 0;

Can you add a comment here what this does? Because it looks like a `&~` operator :)

I also wonder if it would make sense to delegate the `_bits` checks to its own class, for `equal` and `subset`.

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

> 508:   TypeLong::ZERO     = TypeLong::make( 0);   //  0
> 509:   TypeLong::ONE      = TypeLong::make( 1);   //  1
> 510:   TypeLong::NON_ZERO = TypeLong::try_make(TypeIntPrototype<jlong, julong>{{min_jlong, max_jlong}, {1, max_julong}, {0, 0}}, WidenMin)->is_long();

Here we really expect the `try` not to fail, right? Maybe we could have some `make` method, that calls `try_make` and asserts that we succeed?

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

> 1608: const Type* TypeInt::try_make(const TypeIntPrototype<jint, juint>& t, int w, bool dual) {
> 1609:   auto new_t = t.canonicalize_constraints();
> 1610:   if (!new_t._present) {

Here `empty` would read a little more naturally, just an idea.
Ah, and I would rename `new_t` -> `canonical_t` or `canonicalized_t`.

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

> 1618:   return (new TypeInt(TypeIntPrototype<jint, juint>{{lo, lo}, {ulo, ulo}, {~ulo, ulo}},
> 1619:                       WidenMin, false))->hashcons()->is_int();
> 1620: }

We are talking about a constant here, right? In that case, I would rename `lo` -> `con` or `val`.

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

> 1627: const Type* TypeInt::try_make(const TypeIntPrototype<jint, juint>& t, int w) {
> 1628:   return try_make(t, w, false);
> 1629: }

While we are already here: can we use the full name of `w`? Is it `widen`?

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

> 1665:   assert(!_is_dual, "dual types should only be used for join calculation");
> 1666:   const TypeInt* ft = join_helper(kills, include_speculative)->isa_int();
> 1667:   if (ft == nullptr) {

Why can this now not be empty any more?

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

> 1706: 
> 1707: bool TypeInt::empty(void) const {
> 1708:   return false;

Oh, so it can now never be empty, why?

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

> 1724: const TypeLong* TypeLong::TYPE_DOMAIN; // alias for TypeLong::LONG
> 1725: 
> 1726: TypeLong::TypeLong(const TypeIntPrototype<jlong, julong>& t, int w, bool dual)

Rename `w` -> `widen`

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

> 1738: }
> 1739: 
> 1740: const TypeLong* TypeLong::make(jlong lo) {

Rename `lo` -> `con`.

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

PR Review: https://git.openjdk.org/jdk/pull/17508#pullrequestreview-2575570593
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930607348
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930608024
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930614382
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930617991
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930622146
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930626921
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930630031
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930633898
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930642138
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930643070
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930643521