RFR: 8352595: Regression of JDK-8314999 in IR matching

[Christian Hagedorn]

On Fri, 21 Mar 2025 15:41:23 GMT, Marc Chevalier <duke at openjdk.org> wrote:

> A lot of tests for the IR framework used `ALLOC` and friends as a check that would run on the Opto assembly by default, but can also run earlier, but that's no longer the case.
> 
> There were two kinds of tests to fix: the ones rather about `ALLOC`, where the used or expected compile phases have to change, and the tests where `ALLOC` were just a check that would run on opto assembly. For this, I tried to keep the spirit of the test using other regexes made for this stage.

Thanks for addressing this. I have some comments.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPhaseIRMatching.java line 192:

> 190:     @IR(counts = {IRNode.ALLOC, "2", IRNode.ALLOC_OF, "Object", "1"})
> 191:     @ExpectedFailure(ruleId = 5, phase = CompilePhase.BEFORE_MACRO_EXPANSION, counts = 1)
> 192:     public void defaultOnBoth() {

I think for this test you should add a matching on some `PrintOptoAssembly` as well since it tries to verify ideal and opto matching.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/flag/TestCompilePhaseCollector.java line 116:

> 114:                            OPTIMIZE_FINISHED, PRINT_IDEAL);
> 115:         assertContainsOnly(methodToCompilePhases, testClass, "mix8", PHASEIDEALLOOP1, PHASEIDEALLOOP2, FINAL_CODE,
> 116:                            OPTIMIZE_FINISHED, PRINT_IDEAL);

Note that the tests on this file only collect compile phases and actually do not perform any IR matching. So, the IR rules do not need work. This means that for all tests where we use the default compile phase of `ALLOC` (which is `PRINT_OPTO_ASSEMBLY`), we can replace `ALLOC` with just something else that defaults on `PRINT_OPTO_ASSEMBLY`.

This just means that we should not replace `PRINT_OPTO_ASSEMBLY` with `BEFORE_MACRO_EXPANSION` here but instead use a different `IRNode` in the IR rules at these tests that default on `PRINT_OPTO_ASSEMBLY`. I've made some comments further down where this applies. The other changes in the file look fine.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/flag/TestCompilePhaseCollector.java line 222:

> 220:         @Test
> 221:         @IR(failOn = {IRNode.CBNZW_HI})
> 222:         @IR(counts = {IRNode.CBZW_LS, "> 1"})

I've just noticed that these AArch64 specific `IRNode` entries do not have a check that they are only supported on AArch64. In the past, I've added some platform checks for `IRNode` entries only being supported on certain platforms:
https://github.com/openjdk/jdk/blob/e23e0f85ef0f959a68adda0cff9e721ba2173ffc/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java#L2905-L2925

We should probably add a similar check for these `CB*` entries. But that's something that can be done separately. 

For this test here, we only collect compile phases and it does not matter on what platforms these `IRNode` entries are eventually used. So, no action to be done in your changes here.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/flag/TestCompilePhaseCollector.java line 417:

> 415:         @Test
> 416:         @IR(failOn = IRNode.STORE, phase = {PHASEIDEALLOOP1, DEFAULT, PHASEIDEALLOOP2})
> 417:         @IR(counts = {IRNode.LOOP, "3"}, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

Suggestion:

        @IR(counts = {IRNode.FIELD_ACCESS, "3"}, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/flag/TestCompilePhaseCollector.java line 430:

> 428:         @Test
> 429:         @IR(failOn = IRNode.ALLOC, phase = {PHASEIDEALLOOP1, PHASEIDEALLOOP2})
> 430:         @IR(counts = {IRNode.ALLOC, "3"}, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

Suggestion:

        @IR(failOn = IRNode.FIELD_ACCESS, phase = {PHASEIDEALLOOP1, PRINT_OPTO_ASSEMBLY, PHASEIDEALLOOP2})
        @IR(counts = {IRNode.FIELD_ACCESS, "3"}, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/flag/TestCompilePhaseCollector.java line 435:

> 433:         @Test
> 434:         @IR(failOn = IRNode.STORE, phase = {PHASEIDEALLOOP1, PRINT_IDEAL, PHASEIDEALLOOP2})
> 435:         @IR(counts = {IRNode.LOOP, "3"}, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

Suggestion:

        @IR(counts = {IRNode.FIELD_ACCESS, "3"}, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/flag/TestCompilePhaseCollector.java line 460:

> 458:         @Test
> 459:         @IR(counts = {"foo", "3"}, phase = {PHASEIDEALLOOP1, PHASEIDEALLOOP2})
> 460:         @IR(failOn = IRNode.LOOP, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

Suggestion:

        @IR(failOn = IRNode.FIELD_ACCESS, phase = {FINAL_CODE, OPTIMIZE_FINISHED, DEFAULT})

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

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24163#pullrequestreview-2709437094
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009642233
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009705679
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009659733
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009722404
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009723100
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009723308
PR Review Comment: https://git.openjdk.org/jdk/pull/24163#discussion_r2009723545