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

[Kangcheng Xu]

On Mon, 8 Jul 2024 17:24:59 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 23 additional commits since the last revision:
> 
>  - Merge branch 'master' into boolnode-refactor
>  - spread boolean AND and OR into subcases, update number of expected CMP_U nodes
>  - Merge branch 'master' into boolnode-refactor
>  - 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
>  - ... and 13 more: https://git.openjdk.org/jdk/compare/460de6e1...29655d35

I sincerely apologize for not following up on this timely. This won't happen again.

> [...] change the IR rule to something like 
> `@IR(counts = {IRNode.CMP_U + "\b", "1"}`

First, I assume you mean `... + "\\b"` (double escaped for regex).

Unfortunately this does not work. `IRNode.CMP_U` has a postfix `#_`, making the expression `_#CMP_U#_\\b`.

https://github.com/openjdk/jdk/blob/a15af6998e8f7adac2ded94ef5a47e22ddb53452/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java#L440-L443

Even if we explicitly make it `@IR(counts = {"_#CMP_U\\b#_", "1"}`, it is still the one without the `\b` registered by `beforeMatchingNameRegex(irNodePlaceholder, irNodeRegex)`, leading to unexpected node type during assertion:


Violations (4)
--------------
 - IR Node "_#CMP_U\b#_" defined in class IRNode has no regex/compiler phase mapping (i.e. no static initializer block that adds a mapping entry to IRNode.IR_NODE_MAPPINGS).
   Have you just created the entry "_#CMP_U\b#_" in class IRNode and forgot to add a mapping?
   Violation for IR rule 1 at public static boolean compiler.c2.gvn.TestBoolNodeGVN.testShouldHaveCpmUCase1(int,int).
 - [repeated violations omitted]


I think it's the second argument to `beforeMatchingNameRegex(irNodePlaceholder, irNodeRegex)` you want to add the word break to. Not the placeholder. This can only be done in `IRNode.java`. I propose the following change:


 public static final String CMP_U = PREFIX + "CMP_U" + POSTFIX; 
 static { 
-     beforeMatchingNameRegex(CMP_U, "CmpU"); 
+     beforeMatchingNameRegex(CMP_U, "CmpU\\b"); 
 } 


The three existing tests currently referencing `IRNode.CMP_U`: `compiler.c2.irTests.CmpUWithZero`, `compiler.intrinsics.TestCompareUnsigned`, `compiler.c2.irTests.TestUnsignedComparison`, are all passing w/o this change. It does not break existing tests.

I'm going to push a commit to do so. If you think it's not appropriate to change `IRNode.java` with the scope of this issue, I can revert it.

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

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