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

[Quan Anh Mai]

On Mon, 5 May 2025 06:27:14 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:
>> 
>>   alignment wording
>
> src/hotspot/share/opto/rangeinference.cpp line 366:
> 
>> 364:     // We start with all bits where lo[x] == zeros[x] == 0:
>> 365:     //           0 1 1 0 0 0 0 1
>> 366:     U either = lo | bits._zeros;
> 
> I can let this one slide, but the code gives you exactly the negation of what the text describes. The reader might be confused about this, and have to figure out that the bits are all inverted. Not horrible, but not hard to fix either. Up to you.

Nice catch, I changed it to `neither` and do the `not` here.

> src/hotspot/share/opto/rangeinference.cpp line 370:
> 
>> 368:     // lo[x] == zeros[x] == 0. The last one of these bits must be at index i.
>> 369:     //           0 1 1 0 0 0 0 0
>> 370:     U tmp = ~either & find_mask;
> 
> To me a variable name `tmp` smells a little. I prefer expressive names. Up to you :)

I find a name `neither_upto_first_violation` which seems more expressive, I'm not sure if it may raise any confusion.

> src/hotspot/share/opto/rangeinference.cpp line 379:
> 
>> 377:     // In our example, i == 2
>> 378:     //           0 0 1 0 0 0 0 0
>> 379:     U alignment = tmp & (-tmp);
> 
> This line is still magic. Most compiler devs I know do not see this as "standard" math. Could be nice to at least refer to something one could find online on this.
> 
> I did sleep over it and had a proof in mind:
> What do we know about `-tmp`? It cannot have any bits after the last bit set in `bit`, otherwise those bits would not zero out in `tmp + -tmp`. `-tmp` must have the same last bit set as `tmp`, otherwise it would not cancle out. The addition of those bits create a carry bit, that must be cancled out all the way up. This means that the bits before that last bit must be set either exactly in `tmp` or in `-tmp`, but certainly not in both, otherwise the carry bit would not be cancled away. Hence, only that last bit remains in `tmp & (-tmp)`.

I refer to the x86 `blsi` instruction, which does exactly this. The doc also says that it does `(-SRC) bitwiseAND (SRC)`.

https://www.felixcloutier.com/x86/blsi

> src/hotspot/share/opto/rangeinference.cpp line 480:
> 
>> 478:   if (h < ~bounds._hi) {
>> 479:     return AdjustResult<RangeInt<U>>::make_empty();
>> 480:   }
> 
> Another nit: I feel like the "overflow" case here would not have to spill outside of `adjust_lo`.
> And `Optional` style return value would make more sense for the reader at this point, then the reader does not have to worry about why we do a comparison here, and does not have to dive deeper into `adjust_lo`.
> 
> I leave this up to you though.

That's a good point, however I think I will do this later as we don't have an `Optional` in Hotspot yet.

> src/hotspot/share/utilities/intn_t.hpp line 44:
> 
>> 42: // Implementation-wise, this class currently only supports 0 < nbits <= 8. Also
>> 43: // note that this class is implemented so that overflows in alrithmetic
>> 44: // operations are well-defined and wrap-around.
> 
> Suggestion:
> 
> // operations are well-defined and wrap-around, just like jint, juint, jlong and julong.

Overflow in `jint` and `jlong` is actually UB.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2073291807
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2073292416
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2073293867
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2073294529
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r2073296022