8367341: C2: apply KnownBits and unsigned bounds to And / Or operations by merykitty · Pull Request #27618 · openjdk/jdk

merykitty · 2025-10-03T06:07:50Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8367341: C2: apply KnownBits and unsigned bounds to And / Or operations (Enhancement - P4)

Reviewers

Hannes Greule (@SirYwell - Committer)
Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27618/head:pull/27618
$ git checkout pull/27618

Update a local copy of the PR:
$ git checkout pull/27618
$ git pull https://git.openjdk.org/jdk.git pull/27618/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27618

View PR using the GUI difftool:
$ git pr show -t 27618

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27618.diff

Using Webrev

Link to Webrev Comment

…ral Value inferences

bridgekeeper · 2025-10-03T06:08:58Z

👋 Welcome back qamai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-10-03T06:09:00Z

@merykitty This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: hgreule, epeter

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 42 new commits pushed to the master branch:

00050f8: 8373502: C2 SuperWord: speculative check uses VPointer variable was pinned after speculative check, leading to bad graph
b446262: 8373682: Test compiler/loopopts/superword/TestReinterpretAndCast.java fails on x86_64 with AVX but without f16c
0146077: 8373723: Deadlock with JvmtiTagMap::flush_object_free_events()
... and 39 more: https://git.openjdk.org/jdk/compare/78c2d57259ad829a2cfc1370efbb2a5913df4661...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-10-03T06:09:48Z

@merykitty The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-10-03T06:13:17Z

Webrevs

SirYwell

Nice change overall.
I'm not sure how "easily" we can really see the benefit in the example of the interval splitting, but I leave that to others to judge.

I was just wondering, do you think it makes sense to move more such code into the RangeInference classes in future (e.g., for shift ops) or how we'll tell what to place where. From what it looks like the main reason currently is to use the TypeIntMirror classes for testability, which other node types definitely could benefit from as well.

merykitty · 2025-10-06T14:40:40Z

I'm not sure how "easily" we can really see the benefit in the example of the interval splitting, but I leave that to others to judge.

Without it, the simple inference function fails AndLNodeIdealizationTest because the current version also splits the analysis between the negative part and the non-negative part.

I was just wondering, do you think it makes sense to move more such code into the RangeInference classes in future (e.g., for shift ops) or how we'll tell what to place where. From what it looks like the main reason currently is to use the TypeIntMirror classes for testability, which other node types definitely could benefit from as well.

Yes that is entirely my intention, that for example, we only need to implement RangeInference::infer_left_shift and the unittest can be a simple:

class OpLeftShift;
class InferLeftShift:

TEST(opto, range_inference) {
  test_binary<OpLeftShift, InferLeftShift>();
}

merykitty · 2025-10-13T11:07:05Z

@eme64 I think it would be great if you take a look at this PR.

eme64

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

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.

OrINode::add_ring

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

Seems also ok, handled by the KnownBits.

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.

xor_upper_bound_for_ranges
I think also this should be handled by doing KnownBits first, and then inferring the signed/unsigned bounds, right?
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

eme64 · 2025-10-13T12:31:54Z

src/hotspot/share/opto/rangeinference.hpp

+  // These allow TypeIntMirror to mimick the behaviors of TypeInt* and TypeLong*, so they can be
+  // passed into RangeInference methods. These are only used in testing, so they are implemented in
+  // the test file.
+  const TypeIntMirror* operator->() const;
+  TypeIntMirror meet(const TypeIntMirror& o) const;
+  bool contains(U u) const;
+  bool contains(const TypeIntMirror& o) const;
+  bool operator==(const TypeIntMirror& o) const;


Could we limit this to DEBUG_ONLY?

Maybe, it disables these gtest in product builds, however. What do you think?

Right, I see. I suppose we can keep it. Can you somehow make it clear which block it is, maybe with some start/end markers?

I was wondering if the method below is still part of it, but I don't think so. But it was unclear at first.

src/hotspot/share/opto/rangeinference.hpp

eme64 · 2025-10-13T13:17:42Z

test/hotspot/gtest/opto/test_rangeinference.cpp

+#include <array>
+#include <limits>
+#include <type_traits>
+#include <unordered_set>


I don't know the current state of code style guide: but are we allowed to use std::unordered_set?

I can't think of a better way, we have HashTable but it is terrible since the table size is fixed.

Not sure. I'll ask some folks who might know / have an anser / opinion ;)

It surely would be very easy, and not affect the product. But let's see what they say.

They tell me it is fine, and we are already doing similar things here:
test/hotspot/gtest/jfr/test_networkUtilization.cpp

Not a review, just a drive-by comment, following up on @eme64 "They tell me its fine".

I do not think it's okay to use most standard library headers. Doing so can run into issues with things
like our forbidden function mechanism, assert macro collision, and others. My opinion is the uses in
jfr/test_networkUtilization.cpp shouldn't be there, and aren't actually necessary. I just did a spot check,
and the only "good" case I found is test_codestrings.cpp using <regex>, where there isn't any similar
functionality available in hotspot. The suggestion in the discussion @eme64 for a set is RBTree. The O(1)
lookup by a hashtable is unlikely to matter to a gtest.

There is ongoing work updating our usage (see, for example, https://bugs.openjdk.org/browse/JDK-8369186)
and how to do that in a safe and consistent manner.

Use RBTreeCHeap, if going the RBTree route. It's just the easiest way of using it.

Thanks for your inputs, I have removed the usage of std::unordered_map and replaced it with RBTreeCHeap. Is using std::array here fine?

Hi @merykitty,

I think that we don't use the STL because we run without exceptions and because we want our production data structures to have custom allocators, and history :-). As std::array (AFAIU) is 'just' a typed and sized T*, I think it should be fine, as long as you avoid things that might throw!

test/hotspot/gtest/opto/test_rangeinference.cpp

merykitty · 2025-10-15T09:19:33Z

@eme64 Thanks for your review, I believe I have addressed all of your suggestions.

However, we need to make sure that all cases that you are not deleting are indeed covered.

For this, from the testing POV, all the current idealization tests pass.

From the theoretical POV, let me present it below:

For Xor:

return round_up_power_of_2(U(hi_0 | hi_1) + 1) - 1; // This should be trivially covered by `KnownBits`, since it tries to deal with the highest bits that are known to be 0 in both inputs

For Or:

// If both args are bool, can figure out better types
if ( r0 == TypeInt::BOOL ) {
  if ( r1 == TypeInt::ONE) {
    return TypeInt::ONE; // Trivial, since all bits except the lowest is 0 in both inputs, and the lowest bit is 1 in the second input
  } else if ( r1 == TypeInt::BOOL ) {
    return TypeInt::BOOL; // Trivial, since all bits except the lowest is 0 in both inputs
  }
} else if ( r0 == TypeInt::ONE ) {
  if ( r1 == TypeInt::BOOL ) {
    return TypeInt::ONE; // Same as above
  }
}

if (r0 == TypeInt::MINUS_1 || r1 == TypeInt::MINUS_1) {
  return TypeInt::MINUS_1; // Trivial, since all bits is 1 in 1 of the inputs
}

// 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() ); // Constant folding is trivial

For And:

// If both types are constants, we can calculate a constant result.
if (r0->is_con() && r1->is_con()) {
  return IntegerType::make(r0->get_con() & r1->get_con()); // Constant folding is trivial
}

// 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); // In this case, both have a single simple interval, and the max of the result (which is the same as the unsigned max) is not larger than the min of either input.
}

// If only one range is positive, the result will range from 0 up to that range's maximum value.
// For the operation 'x & C' where C is a positive constant, the result will be in the range [0..C]. With that observation,
// we can say that for any integer c such that 0 <= c <= C will also be in the range [0..C]. Therefore, 'x & [c..C]'
// where c >= 0 will be in the range [0..C].
if (r0->_lo >= 0) {
  return IntegerType::make(0, r0->_hi, widen); // r0 will have a single simple interval, and the result will be the union of 2 sets both of which have the max being not larger than r0->_hi
}

if (r1->_lo >= 0) {
  return IntegerType::make(0, r1->_hi, widen); // Same as above
}

// At this point, all positive ranges will have already been handled, so the only remaining cases will be negative ranges
// and constants.

assert(r0->_lo < 0 && r1->_lo < 0, "positive ranges should already be handled!");

// As two's complement means that both numbers will start with leading 1s, the lower bound of both ranges will contain
// the common leading 1s of both minimum values. In order to count them with count_leading_zeros, the bits are inverted.
NativeType sel_val = ~MIN2(r0->_lo, r1->_lo);

NativeType min; // This takes into consideration that the result is negative iff both the inputs are negative, then uses the lower bound to infer the leading 1s in that case
if (sel_val == 0) {
  // Since count_leading_zeros is undefined at 0, we short-circuit the condition where both ranges have a minimum of -1.
  min = -1;
} else {
  // To get the number of bits to shift, we count the leading 0-bits and then subtract one, as the sign bit is already set.
  int shift_bits = count_leading_zeros(sel_val) - 1;
  min = std::numeric_limits<NativeType>::min() >> shift_bits;
}

NativeType max;
if (r0->_hi < 0 && r1->_hi < 0) {
  // If both ranges are negative, then the same optimization as both positive ranges will apply, and the smaller hi
  // value will mask off any bits set by higher values.
  max = MIN2(r0->_hi, r1->_hi); // Both ranges are negative, then similar to when r0->_lo >= 0 && r1->_lo >= 0
} else {
  // In the case of ranges that cross zero, negative values can cause the higher order bits to be set, so the maximum
  // positive value can be as high as the larger hi value.
  max = MAX2(r0->_hi, r1->_hi); // Consider the union of the results when inferring from 4 combinations of simple intervals of the inputs. If both simple intervals are in the negative range, the result is negative. Otherwise, the result will be not larger than the upper bound of the simple interval in the non-negative range.
}

return IntegerType::make(min, max, widen);

eme64 · 2025-10-15T12:08:23Z

src/hotspot/share/opto/rangeinference.hpp

+      // The unsigned value of the result of 'and' is always not greater than both of its inputs
+      // since there is no position at which the bit is 1 in the result and 0 in either input


That does not sound correct.

We could have ranges 0..0b1000 for both. But then both values are 0b0010, and so the result is 0b0010, which is a 1 at a position where both uhi values had zeros.

I think you need to talk about leading zeros somehow.

No this is not about the range, but about the value in an operation. I.e. If z = x & y then z u<= x && z u<= y. This leads to the fact that the upper bound of z is not larger than the upper bounds of x and y.

eme64 · 2025-10-15T12:08:45Z

src/hotspot/share/opto/rangeinference.hpp

+      // The unsigned value of the result of 'or' is always not less than both of its inputs since
+      // there is no position at which the bit is 0 in the result and 1 in either input


Same issue here as above

Same here, if z = x | y then z u>= x && z u>= y. This means that the lower bound of z is not smaller than the lower bounds of x and y.

eme64 · 2025-10-15T12:59:04Z

Nice, thanks for all the updates. I responded to some of the points above.

openjdk · 2025-10-15T16:09:39Z

@merykitty hotspot has been added to this pull request based on files touched in new commit(s).

merykitty · 2025-10-15T16:12:04Z

@eme64 I have removed the usage of std::unordered_map as well as added comments explaining the values of all_instances_size. Do you have any other concern?

eme64

Nice, that looks better already. Just looked over the diff, now going to look at the whole patch again.

test/hotspot/gtest/opto/test_rangeinference.cpp

eme64

Looks really good. I'll run some internal testing now...

eme64

@merykitty Thanks for working on this! Especially I'm happy with the extra gtest-ing that we are now able to do on the types. This optimization will be the entry point for many KnownBits optimizations, that is exciting!

This still needs a second thorough review though, since it is not trivial ;)

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

eme64

Still good :)

openjdk · 2025-11-04T09:42:47Z

@merykitty this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout andorxor
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

merykitty · 2025-11-12T10:34:22Z

May I have a second review, please?

merykitty · 2025-12-16T05:26:22Z

May I have a second review for this PR, please?

SirYwell

Not sure if my review counts but I went through the changes again and it looks like all existing inference logic is covered by the more concise and also more powerful new logic.

merykitty · 2025-12-16T09:15:32Z

@SirYwell Thanks very much for your review!
@eme64 I have just merged with master, could you test the patch again, please?

eme64 · 2025-12-16T09:34:20Z

Testing launched 🚀

eme64 · 2025-12-18T07:05:06Z

Tests passed :)

merykitty · 2025-12-18T07:24:39Z

Thanks a lot for your reviews and testing. I'm very glad that this PR took much less time :)

/integrate

openjdk · 2025-12-18T07:25:30Z

@merykitty This pull request has not yet been marked as ready for integration.

merykitty · 2025-12-18T07:27:10Z

@eme64 Please reapprove this PR if you think it can integrate now, thanks.

merykitty · 2025-12-18T07:30:32Z

/integrate

openjdk · 2025-12-18T07:31:09Z

Going to push as commit e678050.
Since your change was applied there have been 42 commits pushed to the master branch:

00050f8: 8373502: C2 SuperWord: speculative check uses VPointer variable was pinned after speculative check, leading to bad graph
b446262: 8373682: Test compiler/loopopts/superword/TestReinterpretAndCast.java fails on x86_64 with AVX but without f16c
0146077: 8373723: Deadlock with JvmtiTagMap::flush_object_free_events()
... and 39 more: https://git.openjdk.org/jdk/compare/78c2d57259ad829a2cfc1370efbb2a5913df4661...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-18T07:31:18Z

@merykitty Pushed as commit e678050.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Improve Value inferences of And, Or, Xor and implement gtest for gene…

46ce95f

…ral Value inferences

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 3, 2025

openjdk bot added the rfr Pull request is ready for review label Oct 3, 2025

SirYwell reviewed Oct 6, 2025

View reviewed changes

eme64 reviewed Oct 13, 2025

View reviewed changes

Emanuel's reviews

b73850d

eme64 reviewed Oct 15, 2025

View reviewed changes

remove unordered_map, add some comments for all_instances_size

413c4d3

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 15, 2025

remove std::hash

513e3e9

eme64 reviewed Oct 28, 2025

View reviewed changes

test/hotspot/gtest/opto/test_rangeinference.cpp Show resolved Hide resolved

eme64 reviewed Oct 28, 2025

View reviewed changes

eme64 approved these changes Oct 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 29, 2025

Add assertion for the helper in CTPComparator

bd617d7

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Oct 30, 2025

eme64 approved these changes Nov 4, 2025

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 4, 2025

Merge branch 'master' into andorxor

5ab8235

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 4, 2025

merykitty added 2 commits November 12, 2025 11:11

Merge branch 'master' into andorxor

fe182e5

Merge branch 'master' into andorxor

df3df73

SirYwell approved these changes Dec 16, 2025

View reviewed changes

Merge branch 'master' into andorxor

a15ef18

eme64 approved these changes Dec 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 18, 2025

openjdk bot added the integrated Pull request has been integrated label Dec 18, 2025

openjdk bot closed this Dec 18, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 18, 2025

merykitty deleted the andorxor branch January 13, 2026 04:30

		// The unsigned value of the result of 'and' is always not greater than both of its inputs
		// since there is no position at which the bit is 1 in the result and 0 in either input

		// The unsigned value of the result of 'or' is always not less than both of its inputs since
		// there is no position at which the bit is 0 in the result and 1 in either input

Conversation

merykitty commented Oct 3, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 3, 2025

Uh oh!

openjdk bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

SirYwell left a comment

Choose a reason for hiding this comment

Uh oh!

merykitty commented Oct 6, 2025

Uh oh!

merykitty commented Oct 13, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimbarrett Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

merykitty commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merykitty commented Oct 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 3, 2025 •

edited

Loading

openjdk bot commented Oct 3, 2025 •

edited

Loading

mlbridge bot commented Oct 3, 2025 •

edited

Loading

kimbarrett Oct 15, 2025 •

edited

Loading