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

[Quan Anh Mai]

On Wed, 4 Sep 2024 08:36:01 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:
>> 
>>   move static_asserts
>
> src/hotspot/share/opto/compile.cpp line 4485:
> 
>> 4483:     index_max = sizetype->_hi - 1;
>> 4484:   }
>> 4485:   const TypeInt* iidxtype = TypeInt::make(0, index_max, Type::WidenMax)->is_int();
> 
> Can you explain this change, and the new condition `sizetype->_hi > 0`?

If `sizetype->_hi == 0`, this is a load from an empty array and is dead anyway. `sizetype->_hi > 0` ensures that we will have a sane index.

> src/hotspot/share/opto/compile.hpp line 928:
> 
>> 926:   // Workhorse function to sort out the blocked Node_Notes array:
>> 927:   Node_Notes* locate_node_notes(GrowableArray<Node_Notes*>* arr,
>> 928:                                 int idx, bool can_grow = false);
> 
> What was wrong with the `inline`?

I'm not sure but it fails to compile with `inline`, which makes sense because normally an inline function needs its definition at the same place.

> src/hotspot/share/opto/rangeinference.cpp line 50:
> 
>> 48: // Try to tighten the bound constraints from the known bit information
>> 49: // E.g: if lo = 0 but the lowest bit is always 1 then we can tighten
>> 50: // lo = 1
> 
> Add an example like this:
> 
> lo = 2, hi = 9
> zeros = 1111
> ones  = 1100
> -> 4-aligned
> 
>         0    1    2    3    4    5    6    7    8    9    10
>         0000 0001 0010 0011 0100 0101 0110 0111 1000 1001 1010
> bits:   ok   .    .    .    ok   .    .    .    ok   .    .
> bounds:           lo                                 hi
> adjust:           --------> lo                  hi <---

Thanks, that is a great example.

> src/hotspot/share/opto/rangeinference.cpp line 55:
> 
>> 53: adjust_bounds_from_bits(const RangeInt<U>& bounds, const KnownBits<U>& bits) {
>> 54:   // Find the minimum value that is not less than lo and satisfies bits
>> 55:   auto adjust_lo = [](U lo, const KnownBits<U>& bits) {
> 
> I'm not ok with a lambda that is larger than the body of its enclosing function. We also have no benefit here from using a lambda, other than hiding it. But we could better model this with a class that has public and private methods.

Sure, I have moved it to a `static` method.

> src/hotspot/share/opto/rangeinference.cpp line 108:
> 
>> 106:   if (new_hi > bounds._hi) {
>> 107:     return {true, false, {}};
>> 108:   }
> 
> I need a proof that this is ok.

Please let me know if the comment does not persuade you.

> src/hotspot/share/opto/rangeinference.cpp line 139:
> 
>> 137: template <class U>
>> 138: static SimpleCanonicalResult<U>
>> 139: normalize_constraints_simple(const RangeInt<U>& bounds, const KnownBits<U>& bits) {
> 
> What is the difference between "canonical" and "normalized"?

I forgot this one, fixed now.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1744292281
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1744293044
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1744295532
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1744297138
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1744296535
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1744297476