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

Fri Apr 16 01:33:37 UTC 2021

On Thu, 15 Apr 2021 13:19:56 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:
> 
>   Adjust whitelist

Hi Christian,

here is the 1st portion of my comments. I'll return to reviewing it 1st thing tomorrow...

Cheers,
-- Igor

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 107:

> 105:      */
> 106:     public static final Set<String> JTREG_WHITELIST_FLAGS = new HashSet<>(
> 107:             Arrays.asList(

it doesn't seem to be a comprehensive list of flags that don't affect IR verification work, e.g. different GCs. I think it might be easy to go with the blacklist instead, and possibly warning people if there are any flags in `test.*.opts`.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 146:

> 144:     private List<Class<?>> helperClasses = null;
> 145:     private List<Scenario> scenarios = null;
> 146:     private final List<String> flags = new ArrayList<>();

why did you decide to eagerly create list for `flags`, but not for `scenarios` / `helpersClasses`?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 166:

> 164:         if (VERBOSE) {
> 165:             System.out.println("Test class: " + testClass);
> 166:         }

this c-tor can be replaced by:
Suggestion:

        this(StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE).getCallerClass());

and actually, I don't see it being used (outside of the tests for this testlibrary) and I don't think it will ever be used by actual tests that would use this framework, so I think we can safely remove it.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 211:

> 209:     }
> 210: 
> 211:     /**

I'm not sure how much of the benefits all these different `run.*` bring. I fully understand the desire to simplify the most common use-case (i.e. no-arg `run`), but for the rest I'd assume the users will be capable of writing, for example, `new TestFramework(testClass).addScenarios(scenarios).start();` instead of `TestFramework .runWithScenarios(testClass, scenarios);`

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 269:

> 267:      * <ul>
> 268:      *     <li><p>If a helper class is not in the same file as the test class, make sure that JTreg compiles it by using
> 269:      *     {@literal @}compile in the JTreg header comment block.</li>

you don't need `@compile` to compile a helper class, 1st of all, you shouldn't use `@compile` when you want to compile a class in your test, you should use `@build`, 2nd, in this particular case, the class will be automatically built as part of your test b/c jtreg (or rather javac) will be able to statically detect it as a dependency of the code that calls `runWithHelperClasses(Class, Class...)`

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 366:

> 364:         }
> 365: 
> 366:         for (Class<?> helperClass : helperClasses) {

nit: I'd use `var` here

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 367:

> 365: 
> 366:         for (Class<?> helperClass : helperClasses) {
> 367:             TestRun.check(!this.helperClasses.contains(helperClass), "Cannot add the same class twice: " + helperClass);

won't it be easy to use `Set` to store `helperClasses`?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 407:

> 405:                 start(null);
> 406:             } catch (TestVMException e) {
> 407:                 System.err.println("\n" + e.getExceptionInfo());

you shouldn't use "\n", as it might not be the right line-separator. you can either do:
Suggestion:

                System.err.println();
                System.err.println(e.getExceptionInfo());

or
Suggestion:

                System.err.printf("%n%s%n", e.getExceptionInfo());

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 604:

> 602:      * Disable IR verification completely in certain cases.
> 603:      */
> 604:     private void maybeDisableIRVerificationCompletely() {

nit: 
Suggestion:

    private void disableIRVerificationIfNotFeasible() {

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 617:

> 615:                                    "and PrintOptoAssembly), running with -Xint, or -Xcomp (use warm-up of 0 instead)");
> 616:                 return;
> 617:             }

I'd reorder them as "platform" checks are faster than `hasIRAnnotations` check and, I'd guess, will be a more common culprit to disable IR verification.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 669:

> 667:             }
> 668:             if (e instanceof IRViolationException) {
> 669:                 IRViolationException irException = (IRViolationException) e;

nit:
Suggestion:

            if (e instanceof IRViolationException irException) {

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 674:

> 672:                                    + "Compilation(s) of failed matche(s):");
> 673:                 System.out.println(irException.getCompilations());
> 674:                 builder.append(errorMsg).append("\n").append(irException.getExceptionInfo()).append(e.getMessage());

as `builder.toString` will be printed out to cout/cerr, you should use platform-specific line-separator instead of `\n`

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

Changes requested by iignatyev (Reviewer).

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