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

[Christian Hagedorn]

On Fri, 29 Aug 2025 13:17:48 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> # 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 su...

> Hi @chhagedorn, thank you for the additional work and your insights. This is much appreciated from a learner perspective.

Sure, you're welcome :-)

> I didn't fully analyze the Test.java you provided yet, but wanted to check if you are aiming to include the missing IGVN notification code as part of this issue (and its corresponding test). Or are you working on an independent issue?

I think you could squeeze that in here as well. With mainline, you probably need a different notification code because we need to add the `Bool` node instead of the `CmpU` node. But with this patch, we only require the `CmpU`. So, I guess it's not worth to fix it separately only to update it again with this patch.

> My availability will be limited as the October CPU approaches, but it will try to find some timeboxes to make TestBoolNodeGVN.java emit the right test cases for gt, ge , and eq

Sounds good, no hurry. Thanks for taking another look to improve the test!

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

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