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

[Quan Anh Mai]

On Mon, 22 Jan 2024 09:54:59 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix tests, add verify
>
> @merykitty I just had a quick look. Thanks for spitting out parts and making it more reviewable that way! Since John Rose is generally excited (https://github.com/openjdk/jdk/pull/15440#issuecomment-1901609719), I'll now put in a bit more effort into reviewing this.
> 
> Thanks for adding some gtests.
> I would really like to see some IR tests, where we can see that this folds cases, and folds them correctly.
> And just some general java-code correctness tests, which test your optimizations in an end-to-end way.
> 
> I have a general concern with the math functions. They have quite a few arguments, often 5-10. And on average half of them are passed as a reference. Sometimes it is hard to immediately see which are the arguments that will not be mutated, and which are return values, and which are both arguments and return values, which are simply further constrained/narrowed etc.
> 
> I wonder if it might be better to have types like:
> 
> SRange<int/long> {lo, hi}
> URange<int/long> {lo, hi}
> KnownBits<int/long> {ones, zeros}
> 
> Make them immutable, i.e. the fields are constant.
> Then as function parameters, you always pass in these as const, and return the new values (possibly in some combined type, or a pair or tuple or whatever).
> 
> I think it would make the code cleaner, have fewer arguments, and a bit easier to reason about when things are immutable.
> 
> Plus, then you can put the range-inference methods inside those classes, you can directly ask such an object if it is empty etc. You could for example have somelthing like:
> `SRange::constrained_with(KnownBits) -> returns SRange`. Basically I'm asking for the code to be a little more object-oriented, and less C-style ;)

@eme64 Thanks a lot for your reviews, I hope that I have addressed your concerns.

Regarding IR tests, I don't think I can come up with any as there is no node taking advantage of the additional information yet.

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

PR Comment: https://git.openjdk.org/jdk/pull/17508#issuecomment-1904576181