RFR: 8287525: Extend IR annotation with new options to test specific target feature.
Christian Hagedorn
chagedorn at openjdk.java.net
Fri Jun 3 12:03:43 UTC 2022
On Thu, 2 Jun 2022 17:17:21 GMT, Swati Sharma <duke at openjdk.java.net> wrote:
> Hi All,
>
> Currently test invocations are guarded by @requires vm.cpu.feature tags which are specified as the part of test tag specifications. This results into generating multiple test cases if some test points in a test file needs to be guarded by a specific features while others should still be executed in absence of missing target feature.
>
> This is specially important for IR checks based validation since C2 IR nodes creation may heavily rely on existence of specific target feature. Also, test harness executes test points only if all the constraints specified in tag specifications are met, thus imposing an OR semantics b/w @requires tag based CPU features becomes tricky.
>
> Patch extends existing @IR annotation with following two new options:-
>
> - applyIfTargetFeatureAnd:
> Accepts a list of feature pairs where each pair is composed of target feature string followed by a true/false value where a true value necessities existence of target feature and vice-versa. IR verifications checks are enforced only if all the specified feature constraints are met.
> - applyIfTargetFeatureOr: Accepts similar arguments as above option but IR verifications checks are enforced only when at least one of the specified feature constraints are met.
>
> Example usage:
> @IR(counts = {"AddVI", "> 0"}, applyIfTargetFeatureOr = {"avx512bw", "true", "avx512f", "true"})
> @IR(counts = {"AddVI", "> 0"}, applyIfTargetFeatureAnd = {"avx512bw", "true", "avx512f", "true"})
>
> Please review and share your feedback.
>
> Thanks,
> Swati
That's a good feature to have. Thanks for adding support for it! This will definitely simplify the vector IR tests.
test/hotspot/jtreg/compiler/lib/ir_framework/IR.java line 113:
> 111: * IR verifications checks are enforced if any of the specified feature constraint is met.
> 112: */
> 113: String[] applyIfTargetFeatureOr() default {};
I'm not sure if we should follow the existing scheme to also have at least `applyIfTargetFeature` for a single constraint or not. Back there when I've introduced these constraints I was not happy with writing `applyIfAnd/Or` for many tests where I actually did not care about `AND` and `OR`. I guess you can leave it like that and we can come back to this and maybe clean these things up with [JDK-8280120](https://bugs.openjdk.java.net/browse/JDK-8280120) which wants to introduce another attribute to filter based on the architecture.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 122:
> 120: boolean check = hasRequiredFeaturesAnd(irAnno.applyIfTargetFeatureAnd(), "applyIfTargetFeatureAnd");
> 121: if (!check) {
> 122: System.out.println("Disabling IR validation for " + m + ", all feature constraints not met.");
Note that this message will be printed inside the test VM together with a lot of other messages (`-XX:+PrintCompilation` etc.). This output will not be shown by default. So, it will be hard to find this message again and it might not provide an additional value. For printing log messages inside the test VM, you can use the `TestFrameworkSocket` which pipes messages to the JTreg driver VM to print them there. Since the JTreg driver VM only does a minimal printing, it can easily be found again towards the end of the output under `"Messages from Test VM"`. Specify a tag and then you can use it like that:
TestFrameworkSocket.write("Disabling IR matching for " + m + ": Not all feature constraints met.",
"[IREncodingPrinter]", true);
JTreg Output:
STDOUT:
Run Flag VM:
[...]
Messages from Test VM
---------------------
[IREncodingPrinter] Disabling IR matching for test2: Could not match all feature constraints.
[...]
I guess we could use the same kind of messages for the other `applyIf*` methods above. If you want to add them as well, feel free to do so. Otherwise, this could also be done separately in an RFE at some point.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 159:
> 157: if (irAnno.applyIfTargetFeatureAnd().length != 0) {
> 158: applyRules++;
> 159: TestFormat.checkNoThrow((irAnno.applyIfTargetFeatureAnd().length & 1) == 0,
I suggest to use:
Suggestion:
TestFormat.checkNoThrow((irAnno.applyIfTargetFeatureAnd().length % 2) == 0,
instead which seems cleaner. Same for the other check below.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 206:
> 204: }
> 205:
> 206: private boolean hasRequiredFeaturesAnd(String[] andRules, String ruleType) {
`ruleType` is always `applyIfTargetFeatureAnd` and can be replaced as such.
Suggestion for method name: `hasAllRequiredTargetFeatures()` to follow the existing naming scheme.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 210:
> 208: String feature = andRules[i];
> 209: i++;
> 210: String value = andRules[i];
You should trim the user defined strings with `trim()` - just in case. Same for `hasRequiredFeaturesOr()` below.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 213:
> 211: TestFormat.check((value.contains("true") || value.contains("false")), "Incorrect value in " + ruleType + failAt());
> 212: if (!checkTargetFeature(feature, value)) {
> 213: // Rule will not be applied but keep processing the other flags to verify that they are same.
Typo:
Suggestion:
// Rule will not be applied but keep processing the other target features to verify that they are sane.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 214:
> 212: if (!checkTargetFeature(feature, value)) {
> 213: // Rule will not be applied but keep processing the other flags to verify that they are same.
> 214: return false;
You should cache the return value to keep processing the remaining target feature user strings to check for format errors (similar to what you are doing in `hasRequiredFeaturesOr()`). We should probably separate the format checking and the actual evaluation of the values at some point. But that would exceed the scope of this RFE.
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 220:
> 218: }
> 219:
> 220: private boolean hasRequiredFeaturesOr(String[] orRules, String ruleType) {
`ruleType` is always `applyIfTargetFeatureOr` and can be replaced as such.
Suggestion for method name: `hasAnyRequiredTargetFeature()` (I think the related existing `hasNoRequiredFlags()` method should be renamed/refactored to follow that convention as well but that's also for another day).
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 233:
> 231:
> 232: private boolean checkTargetFeature(String feature, String value) {
> 233: String s = WHITE_BOX.getCPUFeatures();
I suggest to name it `cpuFeatures` instead:
Suggestion:
String cpuFeatures = WHITE_BOX.getCPUFeatures();
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 236:
> 234: // Following feature list is in sync with suppressed feature list for KNL target.
> 235: // Please refer vm_version_x86.cpp for details.
> 236: HashSet<String> knlFeatureSet = new HashSet<String>();
The explicit type argument can be removed:
Suggestion:
HashSet<String> knlFeatureSet = new HashSet<>();
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 250:
> 248: knlFeatureSet.add("GFNI");
> 249: knlFeatureSet.add("AVX512_BITALG");
> 250: Boolean isKNLFlagEnabled = (Boolean)WHITE_BOX.getBooleanVMFlag("UseKNLSettings");
The cast is not necessary:
Suggestion:
Boolean isKNLFlagEnabled = WHITE_BOX.getBooleanVMFlag("UseKNLSettings");
test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 257:
> 255: (value.contains("false") && !s.contains(feature))) {
> 256: return true;
> 257: }
Could be simplified to:
Suggestion:
return (value.contains("true") && s.contains(feature)) || (value.contains("false") && !s.contains(feature));
test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/TargetFeatureCheckExample.java line 59:
> 57:
> 58: @Test
> 59: @IR(counts = {"AddVI", "> 0"}, applyIfTargetFeatureAnd = {"avx512bw", "false"})
As this file represents a usage example, please consider using `IRNode.AddVI` by adding a new IR regex to `IRNode`. But it looks like that this test is rather a correctness test for `AddVI` and could be moved to the other vector IR tests. The tests in the `ir_framework.package` are more for providing information about the usage rather than actually testing something meaningful.
But I think it's good to have an example how to use `applyIfTargetFeature*` as well. But maybe such an example would better fit into the existing `IRExample.java` file where we have existing IR examples and descriptions for `applyIf*`.
-------------
Changes requested by chagedorn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8999
More information about the hotspot-compiler-dev
mailing list