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

[Kangcheng Xu]

On Tue, 27 Aug 2024 07:08:25 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.

@chhagedorn It's already pushed. The HEAD has the up to date master merged in. Please let me know how the test goes. Thanks!

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

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