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

[Christian Hagedorn]

On Mon, 26 Aug 2024 17:30:19 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 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/383e93a8...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.

Hi @tabjy, no worries! You're right, you cannot just append the `\\b` as suggested above - this was only possible in an older version of the IR framework. I think going with what you suggested with `beforeMatchingNameRegex(CMP_U, "CmpU\\b")` should do the trick. You can go ahead and push that. Then I can run some internal testing again, just to be sure.

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

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