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

[Emanuel Peter]

On Mon, 8 Jul 2024 16:46:33 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> 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 20 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into boolnode-refactor
>>  - update test values, @run directive, and remove an empty line
>>  - 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
>>  - ... and 10 more: https://git.openjdk.org/jdk/compare/d3817351...715a6304
>
> I pushed a commit to spread test cases compounded with `&` and `|` into subcases to avoid optimizing out semantically equivalent ones and to make test clearer. To make test passing even with `CmpU3` miscounted as `CmpU`, I specified `counts = {IRNode.CMP_U, ">=1"}` instead of strictly `1`. I hope this is acceptable.
> 
> ---
> 
> A case illustrating `CmpU3` matched as `CmpU`:
> 
> 
>     @Test
>     @Arguments(values = {Argument.DEFAULT, Argument.DEFAULT})
>     @IR(counts = {IRNode.CMP_U, "1"},  // <-- expecting strictly 1
>         phase = CompilePhase.AFTER_PARSING,
>         applyIfPlatformOr = {"x64", "true", "aarch64", "true", "riscv64", "true"})
>     public static boolean testShouldHaveCpmUCase1(int x, int m) {
>         return !(Integer.compareUnsigned((x & m), m - 1) > 0);
>     }
> 
> 
> 
> 1) Method "public static boolean compiler.c2.gvn.TestBoolNodeGVN.testShouldHaveCpmUCase1(int,int)" - [Failed IR rules: 1]:
>    * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={AFTER_PARSING}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#CMP_U#_", "1"}, applyIfPlatform={}, failOn={}, applyIfPlatformOr={"x64", "true", "aarch64", "true", "riscv64", "true"}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
>      > Phase "After Parsing":
>        - counts: Graph contains wrong number of nodes:
>          * Constraint 1: "(\d+(\s){2}(CmpU.*)+(\s){2}===.*)"
>            - Failed comparison: [found] 2 = 1 [given]
>              - Matched nodes (2):
> mismatched-->  * 28  CmpU3  === _ 23 27  [[ 39 ]]  !jvms: TestBoolNodeGVN::testShouldHaveCpmUCase1 @ bci:6 (line 93)
>                * 31  CmpU  === _ 23 27  [[ 32 ]]  !jvms: TestBoolNodeGVN::testShouldHaveCpmUCase1 @ bci:9 (line 93)

@tabjy Are you planning to keep working on this?
I talked with @chhagedorn and we would like you to change the IR rule to something like
`@IR(counts = {IRNode.CMP_U + "\b", "1"}`

In a later and separate RFE, we can then adjust the regex for all nodes, in a bulk update.

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

PR Comment: https://git.openjdk.org/jdk/pull/18198#issuecomment-2290761682