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

[Emanuel Peter]

On Wed, 18 Sep 2024 20:29:32 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:
> 
>   add comments, refactor functions to helper class

Wow you are a champ, thanks for the many comments.
Just dropping a first few comments because I'm taking a quick break.

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

> 113:       return {false, {}};
> 114:     }
> 115:   };

Could the fields be const? And the comment above looks like it could be an assert also?

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

> 124: 
> 125: // Various helper functions for TypeInt/TypeLong operations
> 126: class TypeIntHelper {

Why not call it `RangeInference`? After all the file name is `rangeinference.hpp`.

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

> 128:   // Calculate the cardinality of a TypeInt/TypeLong ignoring the bits
> 129:   // constraints, the result is tuned down by 1 to ensure the bottom type is
> 130:   // correctly calculated

So is this an upper bound on the cardinality? And can you explain the "tuned down" part? I'm not following there.

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

> 137:     if (U(srange._lo) == urange._lo) {
> 138:       return urange._hi - urange._lo;
> 139:     }

Here you check that the two ranges are identical? Maybe add an assert that also the `hi` limit is identical? Or did I get something wrong?

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

> 139:     }
> 140: 
> 141:     return (urange._hi - U(srange._lo)) + (U(srange._hi) - urange._lo) + 1;

It looks good, but I don't understand it 🙈 Can you please explain?

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

> 155:     return super->_lo <= sub->_lo && super->_hi >= sub->_hi && super->_ulo <= sub->_ulo && super->_uhi >= sub->_uhi &&
> 156:           (super->_bits._zeros &~ sub->_bits._zeros) == 0 && (super->_bits._ones &~ sub->_bits._ones) == 0;
> 157:   }

why are these not member methods of `CT`? It also looks like this could be split for the components - at least the `_bits` could be a member method of `KnownBits`.

Having it more modular would make it easier to read and understand.

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

PR Review: https://git.openjdk.org/jdk/pull/17508#pullrequestreview-2314571573
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766263310
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766265295
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766267425
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766269589
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766271769
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766274950