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

[Emanuel Peter]

On Tue, 7 May 2024 17:33:29 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 16 additional commits since the last revision:
> 
>  - 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
>  - Merge branch 'master' into boolnode-refactor
>  - update the package name for tests
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/6a68956e...278c436a

VM code looks good, just a few comments about the test. Thanks for adding that one :)

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGVN.java line 2:

> 1: /*
> 2:  * Copyright (c) 2024 Red Hat and/or its affiliates. All rights reserved.

Can you move this to a different directory, please? the `irTests` directory was a good idea when we were just starting with the IR framework, but now it makes more sense to sort tests by "what" is tested, rather than "how". Feel free to put it in the `c2` directory, or even create a new subdirectory like `c2/gvn`.

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGVN.java line 32:

> 30: import compiler.lib.ir_framework.IRNode;
> 31: import compiler.lib.ir_framework.Test;
> 32: import compiler.lib.ir_framework.TestFramework;

Suggestion:

import compiler.lib.ir_framework.*;

I think that would be more concise. Up to you.

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGVN.java line 62:

> 60:                Integer.compareUnsigned((x & m), m + 1) < 0 &
> 61:                Integer.compareUnsigned((m & x), m + 1) < 0;
> 62:     }

I just had an idea: Can you create a few test-cases like `Integer.compareUnsigned((m & x), m + 2) < 0` etc, with IR rules where a `IRNode.CMP_U` is expected? This could just be a sanity check, and see that we have no "off-by-one" errors here. What do you think?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18198#pullrequestreview-2076307485
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1613135415
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1613140304
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1613138930