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

Emanuel Peter epeter at openjdk.org
Mon Oct 13 13:18:45 UTC 2025 

On Fri, 3 Oct 2025 06:07:50 GMT, Quan Anh Mai <qamai 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.

@merykitty Thank you very much for working on this, very exciting. And it seems that the actual logic is now simpler than all the custom logic before!

However, we need to make sure that all cases that you are not deleting are indeed covered.
1. `OrINode::add_ring`

  if ( r0 == TypeInt::BOOL ) {
    if ( r1 == TypeInt::ONE) {
      return TypeInt::ONE;
    } else if ( r1 == TypeInt::BOOL ) {
      return TypeInt::BOOL;
    }
  } else if ( r0 == TypeInt::ONE ) {
    if ( r1 == TypeInt::BOOL ) {
      return TypeInt::ONE;
    }
  }

That seems to be covered by KnownBits.

2. `OrINode::add_ring`

  if (r0 == TypeInt::MINUS_1 || r1 == TypeInt::MINUS_1) {
    return TypeInt::MINUS_1;
  }

Seems also ok, handled by the KnownBits.

3. `OrINode::add_ring`

  // If either input is not a constant, just return all integers.
  if( !r0->is_con() || !r1->is_con() )
    return TypeInt::INT;        // Any integer, but still no symbols.

  // Otherwise just OR them bits.
  return TypeInt::make( r0->get_con() | r1->get_con() );

Constants would also be handeld by KnownBits.

4. `xor_upper_bound_for_ranges`
I think also this should be handled by doing KnownBits first, and then inferring the signed/unsigned bounds, right?

5. `and_value`
Does not look so trivial. Maybe you can go over it step by step, and leave some GitHub code comments?

src/hotspot/share/opto/rangeinference.hpp line 96:

> 94:   KnownBits<U> _bits;
> 95: 
> 96: private:

Did you mean to drop the `private:` here? It also makes other things below public now...

src/hotspot/share/opto/rangeinference.hpp line 152:

> 150: 
> 151:   template <class CTP>
> 152:   static bool int_type_is_equal(const CTP t1, const CTP t2) {

Out of curiosity: why the change `CT*` -> `CTP`?

src/hotspot/share/opto/rangeinference.hpp line 192:

> 190: // inference from the Type infrastructure of the compiler. It also allows more flexibility with the
> 191: // bit width of the integer type. As a result, it is more efficient to use for intermediate steps
> 192: // of inference, as well as more flexible to perform testing on different integer types.

Would have been nice if we could have used the `TypeIntMirror` inside `TypeInt`, i.e. using composition. But sadly, we are already using fields from `TypeInt` directly everywhere, so not sure if that is very nice/easy.

src/hotspot/share/opto/rangeinference.hpp line 199:

> 197:   S _hi;
> 198:   U _ulo;
> 199:   U _uhi;

Why not use `RangeInt`?

src/hotspot/share/opto/rangeinference.hpp line 218:

> 216:   bool contains(U u) const;
> 217:   bool contains(const TypeIntMirror& o) const;
> 218:   bool operator==(const TypeIntMirror& o) const;

Could we limit this to `DEBUG_ONLY`?

src/hotspot/share/opto/rangeinference.hpp line 221:

> 219: 
> 220:   template <class T>
> 221:   TypeIntMirror cast() const;

Can you explain what this casting method is for?

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?

src/hotspot/share/opto/rangeinference.hpp line 301:

> 299:         _current_interval++;
> 300:         return res;
> 301:       }

Do we really need both?

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 ;)

src/hotspot/share/opto/rangeinference.hpp line 353:

> 351:       S<CTP> hi = std::numeric_limits<S<CTP>>::max();
> 352:       U<CTP> ulo = std::numeric_limits<U<CTP>>::min();
> 353:       U<CTP> uhi = MIN2(st1._uhi, st2._uhi);

All lines except this one are trivial. Can you please add a comment about it?
In `and_values` we had comments like below, you'd have to adjust them a little:

  // If both ranges are positive, the result will range from 0 up to the hi value of the smaller range. The minimum
  // of the two constrains the upper bound because any higher value in the other range will see all zeroes, so it will be masked out.
  if (r0->_lo >= 0 && r1->_lo >= 0) {
    return IntegerType::make(0, MIN2(r0->_hi, r1->_hi), widen);
  }

src/hotspot/share/opto/rangeinference.hpp line 365:

> 363:       S<CTP> lo = std::numeric_limits<S<CTP>>::min();
> 364:       S<CTP> hi = std::numeric_limits<S<CTP>>::max();
> 365:       U<CTP> ulo = MAX2(st1._ulo, st2._ulo);

Add a comment about correctness here too.

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

PR Review: https://git.openjdk.org/jdk/pull/27618#pullrequestreview-3331413594
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426192063
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426193545
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426198623
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426205982
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426213040
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426221015
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426234153
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426255498
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426265259
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426289482
PR Review Comment: https://git.openjdk.org/jdk/pull/27618#discussion_r2426292138

More information about the hotspot-compiler-dev mailing list