RFR: 8364970: Redo JDK-8327381 by updating the CmpU type instead of the Bool type [v3]

[Christian Hagedorn]

On Thu, 14 Aug 2025 18:07:53 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:

>> Francisco Ferrari Bihurriet has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Improve the IR test to add the new covered cases
>>    
>>    I also checked the test is now failing in the master branch (at
>>    f95af744b07a9ec87e2507b3d584cbcddc827bbd).
>>  - Remove IR test inverted asserts
>>    
>>    According to my IGV observations, these inversions aren't necessarily
>>    effective. Also, I assume it is safe to remove them because if I apply
>>    this change to the master branch, the test still passes (tested at
>>    f95af744b07a9ec87e2507b3d584cbcddc827bbd).
>>  - Add requested comments from the reviews
>>    
>>    Add a comment with the BoolTest::cc2logical inferences tables, as
>>    suggested by @tabjy.
>>    
>>    Also, add a comment explaining how PhaseCCP::push_cmpu is handling
>>    grandparent updates in the case 1b, as agreed with @chhagedorn.
>
> # Absence note
> 
> Today is the last day before a ~2 weeks vacation, so my next working day is Monday, September 1st.
> 
> Please feel free to keep giving feedback and/or reviews, and I will continue when I'm back.
> 
> Cheers,
> Francisco

Hi @franferrax, hope you had a good vacation!

> Hi @chhagedorn,
> 
> I added the new tests in [e6b1cb8](https://github.com/openjdk/jdk/commit/e6b1cb897d9c75b34744c7d24f72abcec9986b0b). One problem I'm facing is that I'm unable to generate `Bool` nodes with arbitrary `BoolTest` values. Even if I try the assert inversions I removed in [10e1e3f](https://github.com/openjdk/jdk/commit/10e1e3f4f796d05dcd5c56bc2365d5d564d93952), C2 has preference for `BoolTest::ne`, `BoolTest::le` and `BoolTest::lt`. Instead of using `BoolTest::eq`, `BoolTest::gt` or `BoolTest::ge`, it swaps what is put in `IfTrue` and `IfFalse`.
> 
> Even if `javac` generates an `ifeq` and an `ifne` with the same inputs, instead of a single `CmpU` with two `Bool`s (`BoolTest::eq` and `BoolTest::ne`), I get a single `Bool` (`BoolTest::ne`) with two `If` (one of them swapping `IfTrue` with `IfFalse`). I guess this is some sort of canonicalization to enable further optimizations.
> 
> Do you know a way to influence the `Bool`'s `BoolTest` value? Or @rwestrel do you?
> 
> This means the following 8 cases are not really testing what they claim, but repeating other cases with `IfTrue` and `IfFalse` swapped:
> 
> * `testCase1aOptimizeAsFalseForGT(xm|mx)` (they should use `BoolTest::gt`, but use `BoolTest::le`)
> * `testCase1bOptimizeAsFalseForEQ(xm|mx)` (they should use `BoolTest::eq`, but use `BoolTest::ne`)
> * `testCase1bOptimizeAsFalseForGE(xm|mx)` (they should use `BoolTest::ge`, but use `BoolTest::lt`)
> * `testCase1bOptimizeAsFalseForGT(xm|mx)` (they should use `BoolTest::gt`, but use `BoolTest::le`)
> 
> Even if we don't find a way to influence the `BoolTest`, the cases are still valid and can be kept (just in case the described behaviour changes).

Hm, that's a good point. `Parse::do_if()` indeed always canonicalizes the `Bool` nodes... But I was sure we can still somehow end up with non-canonicalized versions again with some tricks. I was curious and played around with some examples and could indeed find test cases for `gt`, `ge` , and `eq`.

I was then also thinking about notification code in IGVN. We already concluded further up that it's not needed for CCP because `CmpU` nodes below `AddI` nodes are put to the worklist again. However, with IGVN, we could modify the graph above the `AndI` as well. We miss notification code for `CmpU` below `AndI`. I changed my test cases further to also run into such a missing optimization case. When run with `-XX:VerifyIterativeGVN=1110`, we indeed get such an assertion failure with the proposed patch (it also triggers an assertion failure already with mainline code). This could be easily fixed with:

diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp
--- a/src/hotspot/share/opto/phaseX.cpp	(revision afa8e79ba1a76066cf969cb3b5f76ea804780872)
+++ b/src/hotspot/share/opto/phaseX.cpp	(date 1756472877934)
@@ -2553,7 +2553,7 @@
   if (use_op == Op_AndI || use_op == Op_AndL) {
     for (DUIterator_Fast i2max, i2 = use->fast_outs(i2max); i2 < i2max; i2++) {
       Node* u = use->fast_out(i2);
-      if (u->Opcode() == Op_RShiftI || u->Opcode() == Op_RShiftL) {
+      if (u->Opcode() == Op_RShiftI || u->Opcode() == Op_RShiftL || u->Opcode() == Op_CmpU) {
         worklist.push(u);
       }
     }

Here are the test cases with some further comments explaining how it works and how to run it:
[Test.java](https://github.com/user-attachments/files/22046942/Test.java)

 This will produce the following IR (at `PhaseIdealLoop1`):
<img width="658" height="167" alt="image" src="https://github.com/user-attachments/assets/293c5348-b4ef-4ae6-bf7d-c429e6002d3d" />
I guess you could easily transform these into IR tests and check that we have 4 `CmoveI/CmpU` nodes in `PhaseIdealLoop1` and then no more in `PhaseIdealLoop2`. What do you think?

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

PR Comment: https://git.openjdk.org/jdk/pull/26666#issuecomment-3237014744