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

[John Tortugo]

On Fri, 23 Apr 2021 12:39:32 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:
> 
>   Review comments by Tobias: Formatting fields, spacing, README.md typos and cleanup

test/lib/jdk/test/lib/hotspot/ir_framework/Run.java line 96:

> 94: public @interface Run {
> 95:     /**
> 96:      * The associated {@link Test @Test} methods (one or more) for for this {@code @Run} annotated run method.

Typo: "for for"

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 149:

> 147:             String testClassName = args[0];
> 148:             System.out.println("Framework main(), about to run tests in class " + testClassName);
> 149:             Class<?> testClass;

Merge this and next line?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 163:

> 161:         Class<?> c;
> 162:         try {
> 163:             c = Class.forName(className);

Maybe just return "Class.forName(className)" here?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 174:

> 172:      */
> 173:     private void addHelperClasses(String[] args) {
> 174:         Class<?>[] helperClasses = getHelperClasses(args);

It's a bit confusing that this local variable has the same name as the class property.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 308:

> 306: 
> 307:     private void processControlAnnotations(Class<?> clazz) {
> 308:         if (!XCOMP) {

Perhaps transform this condition on an early return, i.e., "if (XCOMP) return ;", instead of nesting the whole method under the if.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 340:

> 338:     private void applyClassAnnotations(Class<?> c) {
> 339:         ForceCompileClassInitializer anno = getAnnotation(c, ForceCompileClassInitializer.class);
> 340:         if (anno != null) {

Transform to early return?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 373:

> 371:         boolean exclude = random.nextBoolean();
> 372:         CompLevel level = CompLevel.ANY;
> 373:         if (exclude) {

Transform to early return?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 378:

> 376:                 case 1 -> {
> 377:                     level = CompLevel.C1;
> 378:                     WHITE_BOX.makeMethodNotCompilable(ex, CompLevel.C2.getValue(), false);

I'm curious why two calls (with different values for the last parameter) are needed for these?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java line 591:

> 589:     }
> 590: 
> 591:     private static CompLevel flipCompLevel(CompLevel compLevel) {

Wouldn't be better to put this on the CompLevel enum itself?

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

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