RFR: 8280120: [IR Framework] Add attribute to @IR to enable/disable IR matching based on the architecture

[Christian Hagedorn]

On Wed, 27 Sep 2023 06:12:25 GMT, Daniel Skantz <duke at openjdk.org> wrote:

> This PR adds support for platform features in IR Framework preconditions. This allows us to write platform-specific IR checks in the same .java test file.
> 
> Platforms considered in this PR are: operating system, arch, and data model (32 or 64-bit VM). Supported field values correspond to vm.simpleArch, os.family, and vm.bits, as used in jtreg `@requires` fields. We use the Platform library methods to accomplish this. Otherwise, the new preconditions work similar to the corresponding CPUFeature preconditions.
> 
> Testing: T1-T3, GHA.
> 
> Additional testing: Tweaked SW test succeeds with removed `@requires` field and added IR platform precondition, but fails with just removed `@requires` field on 32-bit Linux. Performed a few spot tests with incorrectly formatted preconditions, and with valid platform checks but invalid counts.

That's a useful addition, thanks for working on this!

I have a couple of comments but otherwise, it looks good!

test/hotspot/jtreg/compiler/lib/ir_framework/IR.java line 116:

> 114:      * IR verifications checks are enforced only if the specified feature constraint is met.
> 115:      */
> 116:     String[] applyIfPlatformFeature() default {};

Maybe we can just use the name `applyIfPlatform`?

test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 121:

> 119: If a `@Test` annotated method has multiple preconditions (for example `applyIf` and `applyIfCPUFeature`), they are evaluated as a logical conjunction. It's worth noting that flags in `applyIf` are checked only if the CPU features in `applyIfCPUFeature` are matched when they are both specified. This avoids the VM flag being evaluated on hardware that does not support it. An example with both `applyIfCPUFeatureXXX` and `applyIfXXX` can be found in [TestPreconditions](../../../testlibrary_tests/ir_framework/tests/TestPreconditions.java) (internal framework test).
> 120: 
> 121: #### Disable/Enable IR Rules based on available Platform Features

Should we just say "platform"?
Suggestion:

#### Disable/Enable IR Rules based on Platform

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

> 65:         "mac",
> 66:         "windows",
> 67:         // vm.simpleArch values

We should also support ppc, arm, and s390

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

> 279:     }
> 280: 
> 281:     private boolean hasAllRequiredPlatformFeature(String[] andRules) {

We should try to clean all these different `hasAll*/hasAny*/check*` up/unify them at some point. But that's for another day.

test/hotspot/jtreg/compiler/loopopts/superword/SumRed_Long.java line 28:

> 26:  * @bug 8076276
> 27:  * @summary Add C2 x86 Superword support for scalar sum reduction optimizations : long test
> 28:  * @requires vm.bits == "64"

Maybe we should file a follow-up RFE to check all IR tests that have such a `@requires` limitation and see if we could just change some specific rules to use these new constraints in order to let the test pass for all platforms.

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

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15938#pullrequestreview-1645735757
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338136087
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338141008
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338132275
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338133272
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338137686