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

[Quan Anh Mai]

On Thu, 19 Sep 2024 09:23:21 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:
>> 
>>   add comments, refactor functions to helper class
>
> src/hotspot/share/opto/rangeinference.cpp line 88:
> 
>> 86:   // different from the corresponding bit in lo, since result is larger than lo
>> 87:   // the bit must be 0 in lo and 1 in result. As result should be the smallest
>> 88:   // value, this bit should be the rightmost one possible.
> 
> The grammar is a bit confusing. Can you please specify which are the lsb and msb? Otherwise right/left/first/... are difficult to interpret.

I found it awkward calling the 2nd significant bit or the 4th least significant bit. As a result, I have added explanations that the value is viewed as a binary string, the first bit would be the msb, the last bit is the lsb, etc.

> 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?

Maybe, but `const` fields in C++ are really strong (it makes the whole object non-assignable), and this is a prototype object that appears briefly so non-const seems fine. Regarding the assert, verifying the canonical-ness is not trivial, so I only do so in the constructor of `TypeInt/TypeLong`, which is probably more important.

> 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`.

While the file is called `rangeinference.hpp`, there has not been any inference yet, these are helpers that are called from `TypeInt/TypeLong` only.

> 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.

This is the cardinality minus 1, which helps avoid overflow calculating the size of the bottom type.

> 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?

Basically `urange` and `srange` can be the same, or their intersection is the union of `[srange._lo, urange._hi]` and `[urange._lo, srange._hi]`. This simply calculates the size of 2 intervals and add them together.

> 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.

They are helper methods that get called from `CT`, the main reason is that `TypeInt` and `TypeLong` are not templates so implementing these would be a huge duplication. This class is used merely to help reduce this duplication.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766926389
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766915973
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766917551
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766918634
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766921978
PR Review Comment: https://git.openjdk.org/jdk/pull/17508#discussion_r1766924130