RFR: 8287525: Extend IR annotation with new options to test specific target feature. [v3]

[Christian Hagedorn]

On Wed, 8 Jun 2022 12:57:31 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
>
> Swati Sharma has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Merge branch 'JDK-8287525' of https://github.com/swati-sha/jdk into JDK-8287525
>  - 8287525: Review comments resolved.

Changes requested by chagedorn (Reviewer).

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 191:

> 189:             applyRules++;
> 190:             TestFormat.checkNoThrow((irAnno.applyIfCPUFeature().length % 2) == 0,
> 191:                                     "Argument count for applyIfCPUFeature should be multiple of two" + failAt());

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.

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 201:

> 199:             applyRules++;
> 200:             TestFormat.checkNoThrow((irAnno.applyIfCPUFeatureOr().length % 2) == 0,
> 201:                                     "Argument count for applyIfCPUFeatureOr should be multiple of two" + failAt());

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.

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 242:

> 240:     }
> 241: 
> 242:     private boolean hasAllRequiredCPUFeature(String[] andRules, String ruleType) {

Paremeter is not used anymore:
Suggestion:

    private boolean hasAllRequiredCPUFeature(String[] andRules) {

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 253:

> 251:     }
> 252: 
> 253:     private boolean hasAnyRequiredCPUFeature(String[] orRules, String ruleType) {

Paremeter is not used anymore:
Suggestion:

    private boolean hasAnyRequiredCPUFeature(String[] orRules) {

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java line 302:

> 300:         // feature is supported by KNL target.
> 301:         if (isKNLFlagEnabled == null ||
> 302:              (isKNLFlagEnabled.booleanValue() && (!knlFeatureSet.contains(feature.toUpperCase()) || falseValue))) {

Boxing is not required:
Suggestion:

             (isKNLFlagEnabled && (!knlFeatureSet.contains(feature.toUpperCase()) || falseValue))) {

test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/TestCPUFeatureCheck.java line 38:

> 36:  */
> 37: 
> 38: public class TestCPUFeatureCheck {

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.

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

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