RFR: 8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests [v6]
Igor Ignatyev
iignatyev at openjdk.java.net
Mon Apr 26 19:09:43 UTC 2021
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
Changes requested by iignatyev (Reviewer).
test/lib/jdk/test/lib/hotspot/ir_framework/AbstractInfo.java line 87:
> 85: String parameters = args == null || args.length == 0 ? "" :
> 86: " with arguments [" + Arrays.stream(args).map(Class::getName).collect(Collectors.joining(",")) + "]";
> 87: throw new TestRunException("Could not find method " + name + " in " + c + parameters);
I'd pass `e` (`NSME`) to `TestRunException::<init>` so the info in it won't be lost
test/lib/jdk/test/lib/hotspot/ir_framework/Argument.java line 80:
> 78: * Float and double values are restricted to the range [-10000,10000].
> 79: */
> 80: RANDOM_EACH
nit: I'd put a trailing comma here, so the future additions will have lesser chance for a merge conflict
test/lib/jdk/test/lib/hotspot/ir_framework/CheckAt.java line 42:
> 40: * and the framework has compiled the associated {@link Test} method.
> 41: */
> 42: COMPILED
nit: trailing comma
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`?
test/lib/jdk/test/lib/hotspot/ir_framework/CompLevel.java line 77:
> 75: * Compilation level 4: C2 compilation with full optimizations.
> 76: */
> 77: C2(4);
nit:
Suggestion:
C2(4),
;
test/lib/jdk/test/lib/hotspot/ir_framework/DeclaredTest.java line 90:
> 88: argumentValString += " (" + (int)(Character)argumentVal + ")";
> 89: }
> 90: builder.append("arg ").append(i).append(": ").append(argumentValString).append(", ");
you can avoid extra string concatination:
Suggestion:
builder.append("arg ").append(i).append(": ").append(argumentVal.toString());
if (argumentVal instanceof Character) {
builder.append(" (").append((int)(Character)argumentVal).append(")");
}
builder..append(", ");
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.
test/lib/jdk/test/lib/hotspot/ir_framework/ForceCompile.java line 44:
> 42: * {@link TestFormatException}.</li>
> 43: * <li><p>{@link CompLevel#WAIT_FOR_COMPILATION}: Does not apply to {@code @ForceCompile} and results in a
> 44: * {@link TestFormatException}.</li>
the same comment as in `ForceCompileClassInitializer.java`
test/lib/jdk/test/lib/hotspot/ir_framework/ForceCompileClassInitializer.java line 44:
> 42: * {@link TestFormatException}.</li>
> 43: * <li><p>{@link CompLevel#WAIT_FOR_COMPILATION}: Does not apply to {@code @ForceCompileClassInitializer} and results in a
> 44: * {@link TestFormatException}.</li>
I don't think you should list all `CompLevel` here as this increases maintenance cost (if/when we update CompLevel) for little to no gain, I'd just mention that `SKIP` and `WAIT_FOR_COMPILATION` are inapplicable here.
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`
test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 54:
> 52:
> 53: public IRMatcher(String hotspotPidFileName, String irEncoding, Class<?> testClass) {
> 54: this.compilations = new HashMap<>();
nite: double space b/w `=` and `new`
test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 127:
> 125: private void parseHotspotPidFile() {
> 126: Map<Integer, String> compileIdMap = new HashMap<>();
> 127: try (BufferedReader br = Files.newBufferedReader(Paths.get(hotspotPidFileName))) {
nit: I'd use `var` here, and it seems this would reduce classes imported from `java.io` to just `IOException`, so we could change L#26 to an explicit import.
Suggestion:
try (var br = Files.newBufferedReader(Paths.get(hotspotPidFileName))) {
test/lib/jdk/test/lib/hotspot/ir_framework/IRNode.java line 132:
> 130:
> 131: static List<String> mergeNodes(String[] nodes) {
> 132: final List<String> mergedNodes = new ArrayList<>();
nit: I don't see any reasons for this var to be `final`.
test/lib/jdk/test/lib/hotspot/ir_framework/RunMode.java line 42:
> 40: * method.
> 41: */
> 42: STANDALONE
nit: a trailing comma
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?
test/lib/jdk/test/lib/hotspot/ir_framework/Scenario.java line 62:
> 60: + "a number: " + SCENARIOS_PROPERTY);
> 61: }
> 62: }
a more idiomatic way to do that would be to remove initialization of `ENABLED_SCENARIOS` at L46 and do:
Suggestion:
if (!SCENARIOS_PROPERTY.isEmpty()) {
var split = SCENARIOS_PROPERTY.split("\\s*,\\s*");
System.out.println(Arrays.toString(split));
try {
ENABLED_SCENARIOS = Arrays.stream(split).map(Integer::parseInt).collect(Collectors.toList());
} catch (NumberFormatException e) {
throw new TestRunException("Provided a scenario index in the -DScenario comma-separated list which is not "
+ "a number: " + SCENARIOS_PROPERTY);
}
} else {
ENABLED_SCENARIOS = Collections.emptyList();
}
test/lib/jdk/test/lib/hotspot/ir_framework/Scenario.java line 66:
> 64: if (!ADDITIONAL_SCENARIO_FLAGS_PROPERTY.isEmpty()) {
> 65: ADDITIONAL_SCENARIO_FLAGS.addAll(Arrays.asList(ADDITIONAL_SCENARIO_FLAGS_PROPERTY.split("\\s*,\\s*")));
> 66: }
similar comment, this can be replaced by just:
Suggestion:
ADDITIONAL_SCENARIO_FLAGS = ADDITIONAL_SCENARIO_FLAGS_PROPERTY.isEmpty() ? Collections.emptyList() : Arrays.asList(ADDITIONAL_SCENARIO_FLAGS_PROPERTY.split("\\s*,\\s*"));
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?
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`
test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 156:
> 154: private List<Scenario> scenarios = null;
> 155: private Set<Integer> scenarioIndices = null;
> 156: private List<String> flags = null;
`null` is a default value for reference types, so there is no need to set it explicitly.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3508
More information about the hotspot-compiler-dev
mailing list