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

[Roberto Castañeda Lozano]

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.

Looks good to me, modulo issues/comments raised by Christian and a few additional minor comments and suggestions.

It seems the IR test framework's annotation-based precondition language is slowly converging towards jtreg's @requires expressions. In the long run, it would be interesting to investigate whether we could simply reuse the jtreg @requires expression support (as suggested in [JDK-8294279](https://bugs.openjdk.org/browse/JDK-8294279)).

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

> 111:     /**
> 112:      * Accepts a single feature pair which is composed of platform feature string followed by a true/false
> 113:      * value where a true value necessities existence of platform feature and vice-versa.

Suggestion:

     * value where a true value necessitates existence of platform feature and vice-versa.

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

> 118:     /**
> 119:      * Accepts a list of feature pairs where each pair is composed of target feature string followed by a true/false
> 120:      * value where a true value necessities existence of target feature and vice-versa.

Suggestion:

     * value where a true value necessitates existence of target feature and vice-versa.

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

> 125:     /**
> 126:      * Accepts a list of feature pairs where each pair is composed of target feature string followed by a true/false
> 127:      * value where a true value necessities existence of target feature and vice-versa.

Suggestion:

     * value where a true value necessitates existence of target feature and vice-versa.

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

> 126:      * Accepts a list of feature pairs where each pair is composed of target feature string followed by a true/false
> 127:      * value where a true value necessities existence of target feature and vice-versa.
> 128:      * IR verifications checks are enforced if any of the specified feature constraint is met.

Suggestion:

     * IR verifications checks are enforced if any of the specified feature constraints is met.

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

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

Maybe rename to `hasAllRequiredPlatformFeatures`?

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPlatformFeatureCheck.java line 64:

> 62:     }
> 63: 
> 64:     // IR rule is enforced if all the feature conditions holds good

Suggestion:

    // IR rule is enforced if all the feature conditions hold good

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPreconditions.java line 110:

> 108:     public static void testApplyBoth4() {}
> 109: 
> 110: }

Maybe you could complete these tests with a couple more tests that check the evaluation order rules w.r.t. VM flag and CPU feature pre-conditions, something like:


    @Test
    @IR(applyIfPlatformFeatureAnd = {"x64", "true", "aarch64", "true"},
        applyIf = {"UseNonExistingFlag", "true"},
        counts = {IRNode.LOOP, ">= 1000"})
    public static void testPlatformFeaturePrecedenceOverVMFlags() {}

    @Test
    @IR(applyIfPlatformFeatureAnd = {"x64", "true", "aarch64", "true"},
        applyIfCPUFeature = {"non-existing-cpu-feature", "true"},
        counts = {IRNode.LOOP, ">= 1000"})
    public static void testPlatformFeaturePrecedenceOverCPUFeatures() {}

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

PR Review: https://git.openjdk.org/jdk/pull/15938#pullrequestreview-1646169084
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338418975
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338419711
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338420066
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338420639
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338422757
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338426470
PR Review Comment: https://git.openjdk.org/jdk/pull/15938#discussion_r1338489724