RFR: 8373555: C2: Optimize redundant input calculations for sign comparisons [v2]

[Manuel Hässig]

On Mon, 5 Jan 2026 08:03:11 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

>> Instead of sign-comparisons with And,Or,Xor,Max,Min nodes, we can directly compare to one of the inputs of the binary nodes if the other input is irrelevant to the comparison.
>> 
>> There are potentially more operations, but these mentioned here are the most obvious ones. Max and Min could theoretically be expanded to arbitrary comparisons to constants, but I didn't want to introduce more complexity for now.
>> 
>> Please let me know what you think :)
>
> Hannes Greule has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update copyright year

Thank you for this neat improvement, @SirYwell! The logic good, but I have some suggestions below.

Out of curiosity: have you thought about writing the test using the Template Framework to maybe get away with a less repetitive test?

Also, I started some testing, since the logic looks sound.

src/hotspot/share/opto/subnode.cpp line 1546:

> 1544: // For comparisons of the form op(a, b) < 0 or op(a, b) >= 0,
> 1545: // it might be enough to compare a < 0 or a >= 0 (or b < 0 or b >= 0) instead.
> 1546: // As a special case, xor requires negating the test

Could you please point out that the trick is to do with the sign bit so the next person does not have to derive it?
Perhaps

Suggestion:

// For comparisons of the form op(a, b) < 0 or op(a, b) >= 0,
// it might be enough to compare a < 0 or a >= 0 (or b < 0 or b >= 0) instead.
// Consider a & b >= 0 for example: if we know a < 0, then we know the sign bit is 1,
// so we only need to check whether b >= 0 to know the result.
// As a special case, xor requires negating the test

src/hotspot/share/opto/subnode.cpp line 1549:

> 1547: // if one argument is known to be negative: -1 ^ b < 0 <==> b >= 0
> 1548: // but not if it is known to be nonnegative: 0 ^ b < 0 <==> b <  0
> 1549: static Node* simplify_sign_invariant_comparison_input(PhaseGVN* phase, BoolNode* bool_node) {

Please add `const` to the local variables and arguments that you are not mutating.

src/hotspot/share/opto/subnode.cpp line 1556:

> 1554:   }
> 1555:   BasicType bt = cop == Op_CmpI ? T_INT : T_LONG;
> 1556:   Node* in_op = cmp->in(1);

Please assert the precondition that `cmp->in(2)` is a zero constant.

test/hotspot/jtreg/compiler/c2/gvn/BoolNodeSimplifySignInvariantTests.java line 85:

> 83: 
> 84:     @Run(test = {
> 85:         "knownAndInputInt1", "knownAndInputInt2", "knownAndInputInt3", "knownAndInputInt4",

For the readability of the test it would be much easier if the methods were not numbered, but named with `Lt` or `Ge` for the comparison and `L` and `R` for which side is pinned to the "interesting" range. That would help a lot when trying to see if the test is correct.

test/hotspot/jtreg/compiler/c2/gvn/BoolNodeSimplifySignInvariantTests.java line 711:

> 709:     }
> 710: 
> 711:     record IntRange(int lo, int hi) {

Do you think this would be useful beyond this test? If so, it would be good to add it to the test library.

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/28782#pullrequestreview-3638337799
PR Review Comment: https://git.openjdk.org/jdk/pull/28782#discussion_r2671453075
PR Review Comment: https://git.openjdk.org/jdk/pull/28782#discussion_r2671684814
PR Review Comment: https://git.openjdk.org/jdk/pull/28782#discussion_r2671394680
PR Review Comment: https://git.openjdk.org/jdk/pull/28782#discussion_r2671651386
PR Review Comment: https://git.openjdk.org/jdk/pull/28782#discussion_r2671701046