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

[Emanuel Peter]

On Sat, 3 May 2025 01:23:18 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 incrementally with one additional commit since the last revision:
> 
>   alignment wording

Just looked over the rest. Wow, now we are very very close, exciting :)

src/hotspot/share/opto/rangeinference.cpp line 480:

> 478:   if (h < ~bounds._hi) {
> 479:     return AdjustResult<RangeInt<U>>::make_empty();
> 480:   }

Another nit: I feel like the "overflow" case here would not have to spill outside of `adjust_lo`.
And `Optional` style return value would make more sense for the reader at this point, then the reader does not have to worry about why we do a comparison here, and does not have to dive deeper into `adjust_lo`.

I leave this up to you though.

src/hotspot/share/opto/rangeinference.cpp line 717:

> 715:                                                   {MAX2(i1->_ulo, i2->_ulo), MIN2(i1->_uhi, i2->_uhi)},
> 716:                                                   {i1->_bits._zeros | i2->_bits._zeros, i1->_bits._ones | i2->_bits._ones}},
> 717:                            MIN2(i1->_widen, i2->_widen), true);

You need to at least indent the `meet` comment. I would also prefer if you had a `else` block and indent there equally, just for optical balance. But I leave that up to you ;)

src/hotspot/share/utilities/intn_t.hpp line 39:

> 37: // nbits == 16 gives a type equivalent to int16_t, and so on. This class may be
> 38: // used to verify the correctness of an algorithm that is supposed to be
> 39: // applicable to all fixed-width integral types. With a few bits, it makes it

Suggestion:

// applicable to all fixed-width integral types. With small nbits, it makes it

src/hotspot/share/utilities/intn_t.hpp line 44:

> 42: // Implementation-wise, this class currently only supports 0 < nbits <= 8. Also
> 43: // note that this class is implemented so that overflows in alrithmetic
> 44: // operations are well-defined and wrap-around.

Suggestion:

// operations are well-defined and wrap-around, just like jint, juint, jlong and julong.

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

PR Review: https://git.openjdk.org/jdk/pull/17508#pullrequestreview-2813916929
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2072911927
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2072925047
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2072935663
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2072936373