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

[Quan Anh Mai]

On Mon, 27 Jan 2025 14:03:17 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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 44 commits:
>> 
>>  - remove precompiled.hpp
>>  - Merge branch 'master' into unsignedbounds
>>  - copyright
>>  - Merge branch 'master' into unsignedbounds
>>  - Merge branch 'master' into unsignedbounds
>>  - move try_cast to Type
>>  - Merge branch 'master' into unsignedbounds
>>  - build failure
>>  - build failures
>>  - whitespace
>>  - ... and 34 more: https://git.openjdk.org/jdk/compare/893d00ac...c4d46c87
>
> src/hotspot/share/opto/rangeinference.cpp line 349:
> 
>> 347: template <class U>
>> 348: static SimpleCanonicalResult<U>
>> 349: canonicalize_constraints_simple(const RangeInt<U>& bounds, const KnownBits<U>& bits) {
> 
> What does the "simple" mean here? To me, what is happening here is not really "simple" if that is what you were trying to say 😆

Added comments, `Simple` here because we consider the positive and negative ranges separately and this process each one of them.

> src/hotspot/share/opto/rangeinference.cpp line 354:
> 
>> 352:     return SimpleCanonicalResult<U>::make_empty();
>> 353:   }
>> 354:   AdjustResult<RangeInt<U>> nbounds{true, true, bounds};
> 
> `nbounds`: what is the `n` here?
> 
> Since you are doing this iteratively, I think `nbits` -> `current_bits`, and `nbounds` -> `current_bounds` would be more explicit.
> 
> Alternatively, you rename the inputs to `old_...`, and just call the ones you keep updating `bits` and `bounds`. Up to you.

It is indeed "new", I renamed it to `canonicalized_bits` and `canonicalized_bounds`

> src/hotspot/share/opto/rangeinference.cpp line 361:
> 
>> 359:     nbounds = adjust_bounds_from_bits(nbounds._result, nbits._result);
>> 360:     if (!nbounds._progress || !nbounds._is_result_consistent) {
>> 361:       return {nbounds._is_result_consistent, nbounds._result, nbits._result};
> 
> That sounds a little scary to me:
> It sounds like we could have inconsistent results: `!nbounds._is_result_consistent` and then simply return the `nbounds._result`. Is that ok, or just confusing wording?
> I think again here `empty` would be our friend: returning an empty result is more sane than returning an inconsistent one 😅

If the `_present` flag is `false` then it does not matter what is in the `result`s.

> src/hotspot/share/opto/rangeinference.hpp line 162:
> 
>> 160:     return super->_lo <= sub->_lo && super->_hi >= sub->_hi &&
>> 161:            super->_ulo <= sub->_ulo && super->_uhi >= sub->_uhi &&
>> 162:            (super->_bits._zeros &~ sub->_bits._zeros) == 0 && (super->_bits._ones &~ sub->_bits._ones) == 0;
> 
> Can you add a comment here what this does? Because it looks like a `&~` operator :)
> 
> I also wonder if it would make sense to delegate the `_bits` checks to its own class, for `equal` and `subset`.

Yes it is indeed a `&~` operator because it is a set subtraction operation on the bitsets.

> src/hotspot/share/opto/type.cpp line 510:
> 
>> 508:   TypeLong::ZERO     = TypeLong::make( 0);   //  0
>> 509:   TypeLong::ONE      = TypeLong::make( 1);   //  1
>> 510:   TypeLong::NON_ZERO = TypeLong::try_make(TypeIntPrototype<jlong, julong>{{min_jlong, max_jlong}, {1, max_julong}, {0, 0}}, WidenMin)->is_long();
> 
> Here we really expect the `try` not to fail, right? Maybe we could have some `make` method, that calls `try_make` and asserts that we succeed?

It is a one-liner so I think making a dedicate API point is unreasonable, normally we don't know `try_make` will return a non-empty set unless the constraints are all constants like in this case.

> src/hotspot/share/opto/type.cpp line 1708:
> 
>> 1706: 
>> 1707: bool TypeInt::empty(void) const {
>> 1708:   return false;
> 
> Oh, so it can now never be empty, why?

A `TypeInt::try_make` that returns an empty type will return `Type::TOP`. I think it is nice that a `TypeInt` object will always have valid bounds as we access them directly.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930855430
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930856967
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930859298
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930860152
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930863636
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1930864237