RFR: 8367341: C2: apply KnownBits and unsigned bounds to And / Or operations

[Quan Anh Mai]

On Mon, 13 Oct 2025 12:40:45 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hi,
>> 
>> This PR improves the implementation of `AndNode/OrNode/XorNode::Value` by taking advantages of the additional information in `TypeInt`. The implementation is pretty straightforward. A clever trick is that by analyzing the negative and positive ranges of a `TypeInt` separately, we have better info for the leading bits. I also implement gtest unit tests to verify the correctness and monotonicity of the inference functions.
>> 
>> Please take a look and leave your reviews, thanks a lot.
>
> src/hotspot/share/opto/rangeinference.hpp line 230:
> 
>> 228: // TypeLong*, or they can be TypeIntMirror which behave similar to TypeInt* and TypeLong* during
>> 229: // testing. This allows us to verify the correctness of the implementation without coupling with
>> 230: // the hotspot compiler allocation infrastructure.
> 
> This sounds a bit like a hack, but maybe a currently necessary one. But it sounds like we are passing something different in the production code vs in gtest testing code, and that's not ideal.
> 
> I suppose an alternative would be to always do the transition from `TypeInt` -> `TypeIntMirror`, before passing it into `RangeInference`. Would that be too much overhead, or have other downsides? I suppose an issue with that is how do you get back a `TypeInt` at the end... yeah not ideal. So maybe your hack is required.
> 
> It would have been nice if we could just compose `TypeIntMirror` inside `TypeInt`, but maybe even that does not solve the whole problem.
> 
> What do you think?

In the strict sense, what is passed in product code and what is passed in gtest will never be the same. This is because `TypeInt` is the set of 32-bit integral values, while we do testing on 3-bit integral values. However, with templates, we can be much more confident since we know that the code being executed for `intn_t<3>` and `jint` is the same one, just specialized with different template parameters. With this approach, I believe we achieve the best similarity between what is executed and what is tested.

> src/hotspot/share/opto/rangeinference.hpp line 324:
> 
>> 322:   static CTP infer_binary(CTP t1, CTP t2, Inference infer) {
>> 323:     CTP res;
>> 324:     bool init = false;
> 
> `init` confused me at first. I intuitively read it as `please_initialize_me`, or the imperative `initialize`! But of course you meant `is_initialized`, right? I would use a longer name to be explicit ;)

You are right, I renamed it to `is_init`.

> test/hotspot/gtest/opto/test_rangeinference.cpp line 250:
> 
>> 248:   static_assert(std::is_same_v<T, TypeIntMirror>);
>> 249:   return *this;
>> 250: }
> 
> We now re-implement these from `TypeIntHelper::int_type_xmeet`. I wonder if we could not at least share some code. Not sure if that is worth it. But having this kind of code duplication opens the risk of divergence and hence bugs.

That's a good idea. I implemented `TypeIntHelper::int_type_union` to do the union.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2431593528
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2431600394
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2431606941