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

Igor Ignatyev iignatyev at openjdk.java.net
Fri Apr 30 19:15:01 UTC 2021

On Tue, 27 Apr 2021 15:37:58 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 two additional commits since the last revision:
>  - Fix XCOMP cases from old framework and turn it into new debug flag -DIgnoreCompilerControls
>  - Apply review comments: Added new Compiler annotation class for @DontCompile, changed C1 into C1_SIMPLE, refactored code for ExcludeRandom and FlipC1C2, added missing flag description in README, and some other smaller refactoring/renamings

Hi Christian,

I've reviewed all the files but `examples` and `tests`, for them I'd actually recommend you to put `examples` into `tests` (possible in a subdir), and just update readme to say that the examples can be found in test directory.

I'd also recommend you to put your java files into separate packages to reflect their decision b/w multiple JVMs (flag, test, driver), so you will end up w/ 3-4 subpackages (+1 for the code shared b/w 3 JVMs), this will make it easier to reason about and maintain the framework.

-- Igor

test/lib/jdk/test/lib/hotspot/ir_framework/RunInfo.java line 89:

> 87:      * {@link #isCompilationSkipped(String)}.
> 88:      *
> 89:      * @return {@code true} if the framework compiled the test;

s/compiled/skipped compilation of/ ?

test/lib/jdk/test/lib/hotspot/ir_framework/RunInfo.java line 105:

> 103:      *
> 104:      * @param testName the test method for which the method object should be returned.
> 105:      * @return {@code true} if the framework compiled the test;

the same comment as above

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

> 982:     private final String hotspotPidFileName;
> 983: 
> 984:     JVMOutput(OutputAnalyzer oa, Scenario scenario, ProcessBuilder process) {

instead of passing a ProcessBuilder, you can either pass a command-line or better just past prepared flags, and make it JVMOutput's (you will need a better name) responsibility to execute the process, create OutputAnalyzer, etc. 


    JVMOutput(List<String> cmds, Scenario scenario) {
    var pb = ProcessTools.createJavaProcessBuilder(cmds);
    try {
            // Calls 'main' of TestFrameworkExecution to run all specified tests with commands 'cmds'.
            // Use executeProcess instead of executeTestJvm as we have already added the JTreg VM and
            // Java options in prepareTestVMFlags().
            this.oa = ProcessTools.executeProcess(process);
        } catch (Exception e) {
            throw new TestFrameworkException("Error while executing Test VM", e);
        this.cmd = pb.command();

and then runTest will be smth like

    private void runTestVM(List<String> additionalFlags) {
          List<String> cmds = prepareTestVMFlags(additionalFlags);
          JVMOutput output = new JVMOutput(cmds, scenario);

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

> 373:             case 2 -> compiler = Compiler.C2;
> 374:             default -> compiler = Compiler.ANY;
> 375:         }


        Compiler compiler =  switch (Utils.getRandomInstance().nextInt() % 3) {
            case 1 -> Compiler.C1;
            case 2 -> Compiler.C2;
            default -> Compiler.ANY;

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

> 794:             if (VERBOSE) {
> 795:                 System.out.println("Run " + test.toString());
> 796:             } else if (testFilterPresent) {

I'm not sure, but it feels like it shouldn't be `else if`

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

> 817:             if (GC_AFTER) {
> 818:                 System.out.println("doing GC");
> 819:                 System.gc();

since this VM has access to WhiteBox API, you can call `WhiteBox.fullGC()` here as it's more "reliable".

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

> 825:             System.out.println(System.lineSeparator() + System.lineSeparator() + "Test execution times:");
> 826:             for (Map.Entry<Long, String> entry : durations.entrySet()) {
> 827:                 System.out.format("%-10s%15d ns" + System.lineSeparator(), entry.getValue() + ":", entry.getKey());

instead of System.lineSeparator(), you can use `%n` in the format string

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

> 958:  * Abstract super class for base, checked and custom run tests.
> 959:  */
> 960: abstract class AbstractTest {

can you move this  and all the classes below into their own .java files? you might want to put them into a separate package (as a way to group/isolate them)

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkPrepareFlags.java line 92:

> 90:                           + String.join(TestFramework.TEST_VM_FLAGS_DELIMITER, flags)
> 91:                           + System.lineSeparator() + TestFramework.TEST_VM_FLAGS_END;
> 92:         TestFrameworkSocket.write(encoding, "flag encoding");

I don't see a need to use socket here, it will significantly simplify the code, the failure analysis, and reproducing if we just save prepared flags into a file w/ a well-known location (e.g. passed as an argument/property to `TestFrameworkPrepareFlags`), and when used command-line argument file (`@-file`) to pass these flags to the test VM.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkSocket.java line 87:

> 85: 
> 86:     /**
> 87:      * Waits for client sockets (created by flag or test VM) to connect. Return the messages received by the clients.

it actually waits for just one client, and return the message sent by it (ore received from):

     * Waits for a client (created by flag or test VM) to connect. Return the messages received from the client.

test/lib/jdk/test/lib/hotspot/ir_framework/TestInfo.java line 57:

> 55:      * allowing a compilation on the requested level in {@link Test#compLevel()}.
> 56:      *
> 57:      * @return {@code true} if the framework compiled the test;

as in `RunInfo`:
s/compiled/skipped compilation of/

test/lib/jdk/test/lib/hotspot/ir_framework/TestVMException.java line 34:

> 32:     TestVMException(String exceptionInfo) {
> 33:         super("There were one or multiple errors. Please check stderr for more information.");
> 34:         this.exceptionInfo = exceptionInfo;

why can't this info be stored as `Throwable::message`?


Changes requested by iignatyev (Reviewer).

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

More information about the hotspot-compiler-dev mailing list