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

[Christian Hagedorn]

On Mon, 26 Apr 2021 18:09:25 GMT, Igor Ignatyev <iignatyev at openjdk.org> wrote:

>> 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/CompLevel.java line 65:
> 
>> 63:      *  Compilation level 1: C1 compilation without any profile information.
>> 64:      */
>> 65:     C1(1),
> 
> this name might be misleading, as one can assume that it means all C1 levels, `C1_SIMPLE`?

I initially had `C1_SIMPLE` but then replaced it with `C1` as `@DontCompile` can only exclude a compilation for a compiler and not a specific level. However, while thinking about it, it might be better to not share `C1/C1_SIMPLE` but use a separate annotation for `@DontCompile`. I added a new `Compiler` annotation class and updated the usages and checks.

> test/lib/jdk/test/lib/hotspot/ir_framework/DontCompile.java line 39:
> 
>> 37:  *     <li><p>The usage of any other compilation level is forbidden and results in a
>> 38:  *            {@link TestFormatException TestFormatException}.</li>
>> 39:  * </ul>
> 
> this actually suggests that we shouldn't use `CompLevel` here and need to introduce another enum to denote compilers.

I was thinking about the same thing while processing an earlier review comment above :) So, it indeed makes sense to use a separate enum for it.

> test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 39:
> 
>> 37: class IRMatcher {
>> 38:     private static final boolean PRINT_IR_ENCODING = Boolean.parseBoolean(System.getProperty("PrintIREncoding", "false"));
>> 39:     private static final Pattern irEncodingPattern =
> 
> we typically use UPPER_CAMEL_CASE for constants such as `irEncodingPattern`

I updated this and other inconsistencies in the naming of `static final` variables.

> test/lib/jdk/test/lib/hotspot/ir_framework/Scenario.java line 55:
> 
>> 53:     static {
>> 54:         if (!SCENARIOS_PROPERTY.isEmpty()) {
>> 55:             System.out.println(Arrays.toString(SCENARIOS_PROPERTY.split("\\s*,\\s*")));
> 
> did you mean to print it here or was it just a leftover from a debugging session?

Good catch. It was a leftover.

> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 135:
> 
>> 133:                                         - To only run the failed tests use -DTest, -DExclude,
>> 134:                                           and/or -DScenarios.
>> 135:                                         - To also get the standard output of the test VM run with\s
> 
> why do you need `\s` here?

I needed this because `jcheck` complained about trailing whitespaces. Maybe this needs to be addressed at some point in `jcheck`.

> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 146:
> 
>> 144:     private static final boolean REPORT_STDOUT = Boolean.getBoolean("ReportStdout");
>> 145:     private final boolean VERIFY_VM = Boolean.getBoolean("VerifyVM") && Platform.isDebugBuild();
>> 146:     private boolean VERIFY_IR = Boolean.parseBoolean(System.getProperty("VerifyIR", "true"));
> 
> it seems both these guys were meant to be `static final`

`VERIFY_VM` should be yes. But `VERIFY_IR` is only initialized and later updated in `disableIRVerificationIfNotFeasible()`. However, I suggest to rename this field into something more meaningful like `irVerificationPossible`.

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

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