-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8287525: Extend IR annotation with new options to test specific target feature. #8999
Conversation
/label add hotspot-compiler-dev |
👋 Welcome back swati-sha! A progress list of the required criteria for merging this PR into |
@swati-sha |
Webrevs
|
/contributor add @jatin-bhateja |
@swati-sha |
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to repeat the same 2 lines.
* 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. | ||
*/ | ||
String[] applyIfTargetFeatureAnd() default {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you used Target
instead of original CPU
(you check output of getCPUFeatures()
only)?
Do you plan to extend this to check flags too in a future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good feature to have. Thanks for adding support for it! This will definitely simplify the vector IR tests.
String value = andRules[i]; | ||
TestFormat.check((value.contains("true") || value.contains("false")), "Incorrect value in " + ruleType + failAt()); | ||
if (!checkTargetFeature(feature, value)) { | ||
// Rule will not be applied but keep processing the other flags to verify that they are same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo:
// Rule will not be applied but keep processing the other flags to verify that they are same. | |
// Rule will not be applied but keep processing the other target features to verify that they are sane. |
} | ||
|
||
@Test | ||
@IR(counts = {"AddVI", "> 0"}, applyIfTargetFeatureAnd = {"avx512bw", "false"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.examples
package are more about 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*
.
if (irAnno.applyIfTargetFeatureAnd().length != 0) { | ||
boolean check = hasRequiredFeaturesAnd(irAnno.applyIfTargetFeatureAnd(), "applyIfTargetFeatureAnd"); | ||
if (!check) { | ||
System.out.println("Disabling IR validation for " + m + ", all feature constraints not met."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -137,6 +154,16 @@ private void checkIRAnnotations(IR irAnno) { | |||
TestFormat.checkNoThrow(irAnno.applyIf().length <= 2, | |||
"Use applyIfAnd or applyIfOr or only 1 condition for applyIf" + failAt()); | |||
} | |||
if (irAnno.applyIfTargetFeatureAnd().length != 0) { | |||
applyRules++; | |||
TestFormat.checkNoThrow((irAnno.applyIfTargetFeatureAnd().length & 1) == 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use:
TestFormat.checkNoThrow((irAnno.applyIfTargetFeatureAnd().length & 1) == 0, | |
TestFormat.checkNoThrow((irAnno.applyIfTargetFeatureAnd().length % 2) == 0, |
instead which seems cleaner. Same for the other check below.
if ((value.contains("true") && s.contains(feature)) || | ||
(value.contains("false") && !s.contains(feature))) { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be simplified to:
if ((value.contains("true") && s.contains(feature)) || | |
(value.contains("false") && !s.contains(feature))) { | |
return true; | |
} | |
return (value.contains("true") && s.contains(feature)) || (value.contains("false") && !s.contains(feature)); |
TestFormat.check((value.contains("true") || value.contains("false")), "Incorrect value in " + ruleType + failAt()); | ||
if (!checkTargetFeature(feature, value)) { | ||
// Rule will not be applied but keep processing the other flags to verify that they are same. | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -176,6 +203,62 @@ private boolean hasAllRequiredFlags(String[] andRules, String ruleType) { | |||
return returnValue; | |||
} | |||
|
|||
private boolean hasRequiredFeaturesAnd(String[] andRules, String ruleType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruleType
is always applyIfTargetFeatureAnd
and can be replaced as such.
Suggestion for method name: hasAllRequiredTargetFeatures()
to follow the existing naming scheme.
} | ||
|
||
private boolean checkTargetFeature(String feature, String value) { | ||
String s = WHITE_BOX.getCPUFeatures(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to name it cpuFeatures
instead:
String s = WHITE_BOX.getCPUFeatures(); | |
String cpuFeatures = WHITE_BOX.getCPUFeatures(); |
return true; | ||
} | ||
|
||
private boolean hasRequiredFeaturesOr(String[] orRules, String ruleType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
* value where a true value necessities existence of target feature and vice-versa. | ||
* IR verifications checks are enforced if any of the specified feature constraint is met. | ||
*/ | ||
String[] applyIfTargetFeatureOr() default {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 which wants to introduce another attribute to filter based on the architecture.
|
TestFormat.checkNoThrow((irAnno.applyIfTargetFeatureOr().length & 1) == 0, | ||
"Argument count for applyIfTargetFeatureOr should be multiple of two" + failAt()); | ||
TestFormat.checkNoThrow((irAnno.applyIfCPUFeatureOr().length % 2) == 0, | ||
"Argument count for applyIfCPUFeatureOr should be multiple of two" + failAt()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format check on L208 cannot be used like that anymore (I can somehow not comment on lines that are hidden):
TestFormat.checkNoThrow(applyRules <= 1,
"Can only specify one apply constraint " + failAt());
We should be allowed to specify one applyIf*
flag constraint together with one applyIfCPUFeature*
constraint while not being allowed to specify multiple applyIf*
flag constraints and/or multiple applyIfCPUFeature*
constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest commenting out check on L208 to enable application of multiple applyIf* constraints OR should that be handled in a separate RFE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it as specifying multiple constraint attributes of the same kind (flag or CPU feature) should not be allowed. But we need separate the count (applyRules
) for the flag based constraints and the CPU feature based constraints. You could rename applyRules
-> flagConstraints
and change the ones belonging to the new CPU features to cpuFeatureConstraints
. Then you can have these checks here, for example:
TestFormat.checkNoThrow(flagConstraints <= 1, "Can only specify one flag constraint" + failAt());
TestFormat.checkNoThrow(cpuFeatureConstraints <= 1, "Can only specify one CPU feature constraint" + failAt());
} | ||
|
||
private boolean hasRequiredFeaturesOr(String[] orRules, String ruleType) { | ||
private boolean hasAnyRequiredCPUFeature(String[] orRules, String ruleType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paremeter is not used anymore:
private boolean hasAnyRequiredCPUFeature(String[] orRules, String ruleType) { | |
private boolean hasAnyRequiredCPUFeature(String[] orRules) { |
@@ -203,37 +239,49 @@ private boolean hasAllRequiredFlags(String[] andRules, String ruleType) { | |||
return returnValue; | |||
} | |||
|
|||
private boolean hasRequiredFeaturesAnd(String[] andRules, String ruleType) { | |||
private boolean hasAllRequiredCPUFeature(String[] andRules, String ruleType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paremeter is not used anymore:
private boolean hasAllRequiredCPUFeature(String[] andRules, String ruleType) { | |
private boolean hasAllRequiredCPUFeature(String[] andRules) { |
return true; | ||
} | ||
if (isKNLFlagEnabled == null || | ||
(isKNLFlagEnabled.booleanValue() && (!knlFeatureSet.contains(feature.toUpperCase()) || falseValue))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unboxing is not required:
(isKNLFlagEnabled.booleanValue() && (!knlFeatureSet.contains(feature.toUpperCase()) || falseValue))) { | |
(isKNLFlagEnabled && (!knlFeatureSet.contains(feature.toUpperCase()) || falseValue))) { |
*/ | ||
|
||
public class TargetFeatureCheckExample { | ||
public class TestCPUFeatureCheck { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a full correctness test on emitting AddVI
, I suggest to move it to the other existing vector IR tests. The tests in ir_framework.examples
and ir_framework.tests
are only executed in tier5 and 6 and are more about testing the framework implementation than the actual correctness of C2 transformations.
TestFormat.checkNoThrow((irAnno.applyIfCPUFeature().length % 2) == 0, | ||
"Argument count for applyIfCPUFeature should be multiple of two" + failAt()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also check that applyIfCPUFeature
is only applied for one constraint and applyIfCPUFeatureAnd
for two or more constraints. This follows the format checks for the flag constraints. This flag constraint design, however, is not very satisfying and has some redundancy. I'm planning to rework this with JDK-8280120.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good to me. Thanks for doing all the updates!
I'll submit some testing.
@swati-sha This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 77 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@chhagedorn, @vnkozlov) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last version looks good.
/integrate |
@swati-sha |
/sponsor |
Going to push as commit 03dca56.
Your commit was automatically rebased without conflicts. |
@jatin-bhateja @swati-sha Pushed as commit 03dca56. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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:-
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.
Example usage:
@ir(counts = {IRNode.ADD_VI, "> 0"}, applyIfCPUFeatureOr = {"avx512bw", "true", "avx512f", "true"})
@ir(counts = {IRNode.ADD_VI, "> 0"}, applyIfCPUFeatureAnd = {"avx512bw", "true", "avx512f", "true"})
Please review and share your feedback.
Thanks,
Swati
Progress
Issue
Reviewers
Contributors
<jbhateja@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8999/head:pull/8999
$ git checkout pull/8999
Update a local copy of the PR:
$ git checkout pull/8999
$ git pull https://git.openjdk.org/jdk pull/8999/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8999
View PR using the GUI difftool:
$ git pr show -t 8999
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8999.diff