RFR: 8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value [v10]

[Emanuel Peter]

On Thu, 30 May 2024 19:35:37 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> This PR resolves [JDK-8327381](https://bugs.openjdk.org/browse/JDK-8327381)
>> 
>> Currently the transformations for expressions with patterns `((x & m) u<= m)` or `((m & x) u<= m)` to `true` is in `BoolNode::Ideal` function with a new constant node of value `1` created. However, this is technically a type-improving (reduction in range) transformation that's better suited in `BoolNode::Value` function.
>> 
>> New unit test `test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java` asserting on IR nodes and correctness of this transformation is added and passing.
>
> Kangcheng Xu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
> 
>  - Merge branch 'master' into boolnode-refactor
>  - move test location, add negative test case, simplify imports
>  - Merge branch 'master' into boolnode-refactor
>  - refactor BoolNode::Value() and extract code to ::Value_cmpu_and_mask
>  - update comments
>  - fix indentation again
>  - apply test only on x64, aarch64 and riscv64
>  - also renames the class name in @run
>  - update test @run annotation
>  - improve formatting, correct annotation and rename test class
>  - ... and 8 more: https://git.openjdk.org/jdk/compare/7bf73e8a...84784c74

Apart from the 2 suggestions, it looks good - thanks for the work!

@chhagedorn told me he would also like to review this. I will also run testing again.

test/hotspot/jtreg/compiler/c2/gvn/TestBoolNodeGVN.java line 33:

> 31:  * @summary Refactor boolean node tautology transformations
> 32:  * @library /test/lib /
> 33:  * @run main compiler.c2.gvn.TestBoolNodeGVN

Suggestion:

 * @run driver compiler.c2.gvn.TestBoolNodeGVN

We only need to pass the flags to the test vm, the driver VM does not need to have external flags.

test/hotspot/jtreg/compiler/c2/gvn/TestBoolNodeGVN.java line 71:

> 69: 
> 70:     private static void testCorrectness() {
> 71:         int[] values = { 0, 1, 5, 8, 16, 42, 100, Integer.MAX_VALUE };

What about adding a random value, and negative nunmbers (i.e. such that they go into the upper range of unsigned)?

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18198#pullrequestreview-2117649601
PR Comment: https://git.openjdk.org/jdk/pull/18198#issuecomment-2167390323
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1639371089
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1639370511