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

[Christian Hagedorn]

On Wed, 26 Jun 2024 06:57:23 GMT, Christian Hagedorn <chagedorn 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/aa824eaf...715a6304
>
> I've run some testing again and the newly added test failed on all platforms in tier2:
> 
> compiler/c2/gvn/TestBoolNodeGVN.java
> 
> Additionally required flags:
> 
> -XX:-TieredCompilation
> 
> Maybe you need to increase the warm-up.
> 
> Output:
> 
> One or more @IR rules failed:
> 
> Failed IR Rules (1) of Methods (1)
> ----------------------------------
> 1) Method "public static boolean compiler.c2.gvn.TestBoolNodeGVN.testShouldHaveCpmU(int,int)" - [Failed IR rules: 1]:
>    * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={AFTER_PARSING}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#CMP_U#_", "4"}, 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 = 4 [given]
>              - Matched nodes (2):
>                * 31  CmpU  === _ 23 27  [[ 32 ]]  !jvms: TestBoolNodeGVN::testShouldHaveCpmU @ bci:9 (line 66)
>                * 60  CmpU  === _ 23 57  [[ 61 ]]  !jvms: TestBoolNodeGVN::testShouldHaveCpmU @ bci:44 (line 68)

> @chhagedorn should we change the regex to have a space at the end, so that we do not do this kind of prefix-matching?

I think that would be a better solution to change the regex. Then we can change the test in such a way that it does an exact counting. Maybe we should move through all the `IRNode` regexes at some point and make sure that all placeholder strings match a unique node. 

We already do that, for example, to match `CountedLoop` and not `CountedLoopEnd` by additionally matching the word boundary of the node name with `\b`:
https://github.com/openjdk/jdk/blob/05d88de05e9b7814ecd5517aacd17f0feafdff3c/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java#L495-L499

We could do something similar for `CmpU` (and we should then probably also apply it for `CmpUL`):

    public static final String CMP_U = PREFIX + "CMP_U" + POSTFIX;
    static {
        String regex = START + "CmpU\\b" + MID + END;
        beforeMatching(CMP_U, regex);
    }

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

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