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

[Emanuel Peter]

On Thu, 1 May 2025 10:48:42 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:
> 
>   Emanuel's reviews

Made it down through `type.cpp`.
Will have to do a pass through the rest later. And then go through the whole thing again.

I feel like we are making good progress on this now :)

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

> 692:     return i1;
> 693:   }
> 694:   const CT* i2 = t2->try_cast<CT>();

And what is the significance of the `dual` state of i1 and `i2`? Should their `dual` state be the same?
Can we add some assert here?

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

> 183: 
> 184:   template <class CT>
> 185:   static const Type* int_type_narrow(const CT* nt, const CT* ot);

Suggestion:

  static const Type* int_type_widen(const CT* new_type, const CT* old_type, const CT* limit_type);

  template <class CT>
  static const Type* int_type_narrow(const CT* new_type, const CT* old_type, const CT* limit_type);

For consistency with its definition.

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

> 1804: bool TypeInt::contains(jint i) const {
> 1805:   juint u = i;
> 1806:   return i >= _lo && i <= _hi && u >= _ulo && u <= _uhi && _bits.is_satisfied_by(u);

Suggestion:

  return _lo  <= i && i <= _hi  &&
         _ulo <= u && u <= _uhi &&
         _bits.is_satisfied_by(u);

Optional, for readability.

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

> 1807: }
> 1808: 
> 1809: bool TypeInt::contains(const TypeInt* t) const {

Is dual allowed here, or could we assert?

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

> 1846:     // The widen bits must be allowed to run freely through the graph.
> 1847:     return (new TypeInt(TypeIntPrototype<jint, juint>{{ft->_lo, ft->_hi}, {ft->_ulo, ft->_uhi}, ft->_bits},
> 1848:                         this->_widen, false))->hashcons();

Why not use `TypeInt::make`?

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

> 1928: bool TypeLong::contains(jlong i) const {
> 1929:   julong u = i;
> 1930:   return i >= _lo && i <= _hi && u >= _ulo && u <= _uhi && _bits.is_satisfied_by(u);

Suggestion:

  return _lo  <= i && i <= _hi  &&
         _ulo <= u && u <= _uhi &&
         _bits.is_satisfied_by(u);

For readability.

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

PR Review: https://git.openjdk.org/jdk/pull/17508#pullrequestreview-2810044167
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2070423848
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2070413425
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2070420492
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2070430474
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2070432896
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2070437258