RFR: 8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests [v5]

Thu Apr 22 13:33:33 UTC 2021

On Tue, 20 Apr 2021 13:52:13 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This RFE provides an IR test framework to perform regex-based checks on the C2 IR shape of test methods emitted by the VM flags `-XX:+PrintIdeal` and `-XX:+PrintOptoAssembly`. The framework can also be used for other non-IR matching (and non-compiler) tests by providing easy to use annotations for commonly used testing patterns and compiler control flags.
>> 
>> The framework is based on the ideas of the currently present IR test framework in [Valhalla](https://github.com/openjdk/valhalla/blob/e9c78ce4fcfd01361c35883e0d68f9ae5a80d079/test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypeTest.java) (mainly implemented by @TobiHartmann) which is being used with great success. This new framework aims to replace the old one in Valhalla at some point.
>> 
>> A detailed description about how this new IR test framework works and how it is used is provided in the [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md) file and in the [Javadocs](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/doc/jdk/test/lib/hotspot/ir_framework/package-summary.html) written for the framework classes.
>> 
>> To finish a first version of this framework for JDK 17, I decided to leave some improvement possibilities and ideas to be followed up on in additional RFEs. Some ideas are mentioned in "Future Work" in [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md) and were also created as subtasks of this RFE.
>> 
>> Testing (also described in "Internal Framework Tests in [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md)):
>> There are various tests to verify the correctness of the test framework which can be found as JTreg tests in the [tests](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/tests) folder. Additional testing was performed by converting all compiler Inline Types test of project Valhalla (done by @katyapav in [JDK-8263024](https://bugs.openjdk.java.net/browse/JDK-8263024)) that used the old framework to the new framework. This provided additional testing for the framework itself. We ran the converted tests with all the flag settings used in hs-tier1-9 and hs-precheckin-comp. For sanity checking, this was also done with a sample IR test in mainline.
>> 
>> Some stats about the framework code added to [ir_framework](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework):
>> 
>> -  without the [Javadocs files](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/doc) : 60 changed files, 13212 insertions, 0 deletions.
>>    - without the [tests](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/tests)  and [examples](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/examples) folder: 40 files changed, 6781 insertions
>>       - comments: 2399 insertions (calculated with `git diff --cached !(tests|examples) | grep -c -E "(^[+-]\s*(/)?*)|(^[+-]\s*//)"`)
>>       - which leaves 4382 lines of code inserted 
>> 
>> Big thanks to:
>> - @TobiHartmann for all his help by discussing the new framework and for providing insights from his IR test framework in Valhalla.
>> - @katyapav for converting the Valhalla tests to use the new framework which found some harder to catch bugs in the framework and also some actual C2 bugs.
>> - @iignatev for helping to simplify the framework usage with JTreg and with the framework internal VM calling structure.
>> - and others who provided valuable feedback.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove Javadocs and README.html, update README.md to reference java files instead of html files

Great work, Christian! I'm glad we finally have the capability to write regression tests with IR verification.

Hard to review this in detail but I've added some comments. In any case, conversion of Valhalla tests to the new framework has proven that this is good to go. We can still fix problems and add new features once this is in.

test/lib/jdk/test/lib/hotspot/ir_framework/Check.java line 94:

> 92: @Retention(RetentionPolicy.RUNTIME)
> 93: public @interface Check {
> 94: 

Some classes/interfaces have a newline at the beginning and others don't. I think they can be removed.

test/lib/jdk/test/lib/hotspot/ir_framework/IREncodingPrinter.java line 36:

> 34: 
> 35: /**
> 36:  * Prints an encoding of all @Test methods whether an @IR rules should be applied to the dedicated test framework socket.

The comment is a bit confusing, maybe "rules" -> "rule"?

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 4:

> 2: This folder contains a test framework whose main purpose is to perform regex-based checks on the C2 IR shape of test methods emitted by the VM flags _-XX:+PrintIdeal_ and _-XX:+PrintOptoAssembly_. The framework can also be used for other non-IR matching (and non-compiler) tests by providing easy to use annotations for commonly used testing patterns and compiler control flags.
> 3: 
> 4: The framework is based on the ideas of the currently present IR test framework in [Valhalla](https://github.com/openjdk/valhalla/blob/e9c78ce4fcfd01361c35883e0d68f9ae5a80d079/test/hotspot/jtreg/runtime/valhalla/inlinetypes/InlineTypesTest.java) and aims to replace it at some point.

I think this line can be removed because we will happily remove the Valhalla version as soon as this one gets integrated :)

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 46:

> 44: 
> 45: #### Checked Tests
> 46: The base tests do not provide any way of verification by user code. A checked test enabled that by allowing the user to define an additional `@Check` annotated method which is invoked directly after the `@Test` annotated method. This allows the user to perform various checks about the test method including return value verification.

"enabled" -> "enables"

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 65:

> 63: A regex can either be a custom string or any of the default regexes provided by the framework in [IRNode](IRNode.java) for some commonly used IR nodes (also provides the possibility of composite regexes).
> 64: 
> 65: An IR verification cannot (and does not want to) always be performed. For example, a JTreg test could be run with _-Xint_ or not a debug build (_-XX:+PrintIdeal_ and _-XX:+PrintOptoAssembly_ are debug build flags). But also CI tier testing could add additional JTreg VM and Javaoptions flags which could make an IR rule unstable. 

Maybe rephrase "does not want to"

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 93:

> 91: - `-DTest=test1,test2`: Provide a list of `@Test` method names which should be executed.
> 92: - `-DExclude=test3`: Provide a list of `@Test` method names which should be excluded from execution.
> 93: - `-DScenarios=1,2`: Provide a list of scenario indexes to specify which scenarios that should be executed.

"that" should be removed

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 101:

> 99: - `-DExcluceRandom=true`: The framework randomly excludes some methods from compilation. IR verification is disabled completely with this flag.
> 100: - `-DFlipC1C2=true`: The framework compiles all `@Test` annotated method with C1 if a C2 compilation would have been applied and vice versa. IR verification is disabled completely with this flag.
> 101: - `-DShuffleTests=false`: Disables the shuffling of tests (a shuffling is always done by default).

Maybe explain that "shuffling" means execution in random order.

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 128:

> 126: This framework is based on the idea of the currently present IR test framework in [Valhalla](https://github.com/openjdk/valhalla/blob/e9c78ce4fcfd01361c35883e0d68f9ae5a80d079/test/hotspot/jtreg/runtime/valhalla/inlinetypes/InlineTypesTest.java). This IR framework was used with great success in Valhalla and thus served as a foundation for this new IR framework.
> 127:  
> 128:  The new framework supports all the features that are present in the Valhalla IR framework with the idea to replace it at some point. The initial design and feature set was kept simple and straight forward and serves well for small to medium sized tests. There are a lot of possibilities to further enhance the framework and make it more powerful. This can be tackled in additional RFEs. A few ideas include:

Again, I would remove the reference to the Valhalla framework. Looking through the changes, I see 13 references to "Valhalla", I think these should all be removed.

test/lib/jdk/test/lib/hotspot/ir_framework/README.md line 135:

> 133: - More interface methods for verification in checked and custom run tests.
> 134: - More stress/debug framework flags.
> 135: - Additional check possibilities in `@IR` annotations. 

I would remove the "Future Work" section and migrate it completely to RFEs. Otherwise it will quickly be outdated anyway.

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3508