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

[Emanuel Peter]

On Sun, 20 Oct 2024 16:41:19 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 32 commits:
> 
>  - Merge branch 'master' into unsignedbounds
>  - address reviews
>  - comment adjust_lo empty case
>  - formality
>  - address reviews
>  - add comments, refactor functions to helper class
>  - refine comments
>  - remove leftover code
>  - add doc to TypeInt, rename parameters, remove unused methods
>  - change (~v & ones) == 0 to (v & ones) == ones
>  - ... and 22 more: https://git.openjdk.org/jdk/compare/309b9291...7f3316fa

Sorry, I've been very slow on this. A few more comments before lunch.

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

> 157: 
> 158:   template <class CT>
> 159:   static bool int_type_subset(const CT* super, const CT* sub) {

Suggestion:

  static bool is_int_type_equal(const CT* t1, const CT* t2) {
    return t1->_lo == t2->_lo && t1->_hi == t2->_hi &&
           t1->_ulo == t2->_ulo && t1->_uhi == t2->_uhi &&
           t1->_bits._zeros == t2->_bits._zeros && t1->_bits._ones == t2->_bits._ones;
  }

  template <class CT>
  static bool is_int_type_subset(const CT* super, const CT* sub) {

I think these should be `is_...` names.

src/hotspot/share/opto/type.hpp line 616:

> 614:  *
> 615:  * 1. Since every TypeInt instance is canonicalized, all the bounds must also
> 616:  * be elements of such TypeInt. Or else, we can tighted the bounds by narrowing

Suggestion:

 * be elements of such TypeInt. Or else, we can tighten the bounds by narrowing

src/hotspot/share/opto/type.hpp line 620:

> 618:  *
> 619:  * 2. Either _lo == jint(_ulo) and _hi == jint(_uhi), or all elements of a
> 620:  * TypeInt lie in the intervals [_lo, jint(_uhi)] or [jint(_ulo), _hi]

The `[_lo, jint(_uhi)] or [jint(_ulo), _hi]` in english is not precise enough.
- Is it a mathematical `OR`: the element can also be in both? In that case I would add "or both".
- Is it a mathematical `XOR`? Then I would write "either ... or .. but not both"

src/hotspot/share/opto/type.hpp line 622:

> 620:  * TypeInt lie in the intervals [_lo, jint(_uhi)] or [jint(_ulo), _hi]
> 621:  *
> 622:  * Proof: For 2 jint value x, y such that they are both >= 0 or < 0. Then:

Suggestion:

 * Proof: For 2 jint value x, y such that they are both >= 0 or both < 0. Then:

Or are you allowing them to one be positive and one negative?

src/hotspot/share/opto/type.hpp line 645:

> 643:  * can be seen that _lo and jint(_uhi) are both < 0 or >= 0, and the same
> 644:  * applies to jint(_ulo) and _hi.
> 645:  */

I would appreciate some indentation: it would make it easier to see points 1, 2, ... And to see what is part of the proof, and what is part of a case distinction and each case in it.

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

PR Review: https://git.openjdk.org/jdk/pull/17508#pullrequestreview-2412481589
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1827379995
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1827388140
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1827391449
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1827393271
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1827397025