RFR: 8364970: Redo JDK-8327381 by updating the CmpU type instead of the Bool type [v3]
Francisco Ferrari Bihurriet
fferrari at openjdk.org
Thu Aug 14 18:10:30 UTC 2025
On Thu, 14 Aug 2025 06:08:22 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:
>> Hi, this pull request is a second take of 1383fec41756322bf2832c55633e46395b937b40, by updating the `CmpUNode` type as either `TypeInt::CC_LE` (case 1a) or `TypeInt::CC_LT` (case 1b) instead of updating the `BoolNode` type as `TypeInt::ONE`.
>>
>> With this approach a56cd371a2c497e4323756f8b8a08a0bba059bf2 becomes unnecessary. Additionally, having the right type in `CmpUNode` could potentially enable further optimizations.
>>
>> #### Testing
>>
>> In order to evaluate the changes, the following testing has been performed:
>>
>> * `jdk:tier1` (see [GitHub Actions run](https://github.com/franferrax/jdk/actions/runs/16789994433))
>> * [`TestBoolNodeGVN.java`](https://github.com/openjdk/jdk/blob/jdk-26+9/test/hotspot/jtreg/compiler/c2/gvn/TestBoolNodeGVN.java), created for [JDK-8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value](https://bugs.openjdk.org/browse/JDK-8327381) (1383fec41756322bf2832c55633e46395b937b40)
>> * I also checked it breaks if I remove the `CmpUNode::Value_cmpu_and_mask` call
>> * Private reproducer for [JDK-8349584: Improve compiler processing](https://bugs.openjdk.org/browse/JDK-8349584) (a56cd371a2c497e4323756f8b8a08a0bba059bf2)
>> * A local slowdebug run of the `test/hotspot/jtreg/compiler/c2` category on _Fedora Linux x86_64_
>> * Same results as with `master` (f95af744b07a9ec87e2507b3d584cbcddc827bbd)
>
> 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.
Learning a bit more about the IR tests framework, I noticed `testCorrectness` isn't probably doing what we want.
It should execute the compiled versions of the following `@Test` methods:
testCase(1a|1b)(OptimizeAsTrue|OptimizeAsFalse)For(EQ|NE|LE|GE|LT|GT)(xm|mx)
But the `@Test` methods warmup only occurs for `TestFramework.run()` and in a different JVM process.
So directly invoking `testCorrectness` outside `TestFramework.run()` is executed in the parent JVM without any warmup.
I think I fixed this in e2f8a43ce1ed2861c506af787018c38ed8769fe3 by making `testCorrectness` a `@Run`ner of those `@Test` methods, but please confirm.
# 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
-------------
PR Comment: https://git.openjdk.org/jdk/pull/26666#issuecomment-3189413611
PR Comment: https://git.openjdk.org/jdk/pull/26666#issuecomment-3189418153
More information about the hotspot-compiler-dev
mailing list