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

danielogh · 2023-09-27T06:12:25Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8280120: [IR Framework] Add attribute to @IR to enable/disable IR matching based on the architecture (Enhancement - P4)

Reviewers

Roberto Castañeda Lozano (@robcasloz - Reviewer)
Emanuel Peter (@eme64 - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15938/head:pull/15938
$ git checkout pull/15938

Update a local copy of the PR:
$ git checkout pull/15938
$ git pull https://git.openjdk.org/jdk.git pull/15938/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15938

View PR using the GUI difftool:
$ git pr show -t 15938

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15938.diff

Webrev

Link to Webrev Comment

…tures before cpu features; lint new test; tweak tests.

bridgekeeper · 2023-09-27T06:14:01Z

👋 Welcome back danielogh! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-27T06:16:06Z

@danielogh The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-09-27T06:20:46Z

Webrevs

01: Full - Incremental (2cfb95f9)
00: Full (e8d15462)

chhagedorn

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

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

chhagedorn · 2023-09-27T06:58:08Z

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java

+        "linux",
+        "mac",
+        "windows",
+        // vm.simpleArch values


We should also support ppc, arm, and s390

chhagedorn · 2023-09-27T06:59:08Z

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java

@@ -233,6 +278,76 @@ private boolean hasAllRequiredFlags(String[] andRules, String ruleType) {
        return returnValue;
    }

+    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.

chhagedorn · 2023-09-27T07:02:04Z

test/hotspot/jtreg/compiler/lib/ir_framework/IR.java

+     * value where a true value necessities existence of platform feature and vice-versa.
+     * IR verifications checks are enforced only if the specified feature constraint is met.
+     */
+    String[] applyIfPlatformFeature() default {};


Maybe we can just use the name applyIfPlatform?

chhagedorn · 2023-09-27T07:03:31Z

test/hotspot/jtreg/compiler/loopopts/superword/SumRed_Long.java

@@ -25,7 +25,6 @@
 * @test
 * @bug 8076276
 * @summary Add C2 x86 Superword support for scalar sum reduction optimizations : long test
- * @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.

chhagedorn · 2023-09-27T07:06:54Z

test/hotspot/jtreg/compiler/lib/ir_framework/README.md

@@ -118,6 +118,11 @@ Sometimes, an `@IR` rule should only be applied if a certain CPU feature is pres

 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).

+#### Disable/Enable IR Rules based on available Platform Features


Should we just say "platform"?

Suggested change

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

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

robcasloz

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).

robcasloz · 2023-09-27T10:45:45Z

test/hotspot/jtreg/compiler/lib/ir_framework/IR.java

@@ -108,6 +108,27 @@
     */
    String[] applyIf() default {};

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


Suggested change

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

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

robcasloz · 2023-09-27T10:46:30Z

test/hotspot/jtreg/compiler/lib/ir_framework/IR.java

+
+    /**
+     * 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.


Suggested change

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

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

robcasloz · 2023-09-27T10:46:49Z

test/hotspot/jtreg/compiler/lib/ir_framework/IR.java

+
+    /**
+     * 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.


Suggested change

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

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

robcasloz · 2023-09-27T10:47:22Z

test/hotspot/jtreg/compiler/lib/ir_framework/IR.java

+    /**
+     * 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 if any of the specified feature constraint is met.


Suggested change

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

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

robcasloz · 2023-09-27T10:49:33Z

test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java

@@ -233,6 +278,76 @@ private boolean hasAllRequiredFlags(String[] andRules, String ruleType) {
        return returnValue;
    }

+    private boolean hasAllRequiredPlatformFeature(String[] andRules) {


Maybe rename to hasAllRequiredPlatformFeatures?

robcasloz · 2023-09-27T10:53:12Z

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPlatformFeatureCheck.java

+        }
+    }
+
+    // IR rule is enforced if all the feature conditions holds good


Suggested change

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

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

robcasloz · 2023-09-27T11:54:25Z

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPreconditions.java

+        applyIfAnd = {"UseSVE", "= 0", "LoopMaxUnroll", "= 0"},
+        counts = {IRNode.LOOP, ">= 1000"})
+    public static void testApplyBoth4() {}
+
 }


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() {}

Good idea! We got something similar wrong recently, so this is a must.

Update: Not necessary

danielogh · 2023-09-29T07:43:43Z

Thanks @chhagedorn for the suggestion. The preconditions read better after renaming. Going over IR tests and removing @requires fields sounds good in order to run tests on all platforms and to consolidate tests.

Appreciate the review @robcasloz. I rewrote parts of the documentation and added more tests. Since we are implementing functionality that is similar to jtreg @requires reusing that implementation could be favorable.

This PR might be ready for another round of reviews.

robcasloz · 2023-09-29T11:50:00Z

Appreciate the review @robcasloz. I rewrote parts of the documentation and added more tests.

The new version looks good. Maybe I missed something, but does any of the newly added tests address my suggestion (https://github.com/openjdk/jdk/pull/15938/files#r1338489724) to check that platform preconditions take precedence over VM flags, i.e. VM flags are not even validated if platform preconditions do not hold?

danielogh · 2023-09-29T12:00:27Z

Appreciate the review @robcasloz. I rewrote parts of the documentation and added more tests.

The new version looks good. Maybe I missed something, but does any of the newly added tests address my suggestion (https://github.com/openjdk/jdk/pull/15938/files#r1338489724) to check that platform preconditions take precedence over VM flags, i.e. VM flags are not even validated if platform preconditions do not hold?

Thanks for the review. The intention is that the last four tests in TestPreconditions.java test precedence through platform-specific VM flags. As an example testApplyPlatformSVE will fail with Could not find VM flag "UseSVE" if we remove the applyIfPlatform precondition on x64.

robcasloz · 2023-09-29T12:11:10Z

Appreciate the review @robcasloz. I rewrote parts of the documentation and added more tests.

The new version looks good. Maybe I missed something, but does any of the newly added tests address my suggestion (https://github.com/openjdk/jdk/pull/15938/files#r1338489724) to check that platform preconditions take precedence over VM flags, i.e. VM flags are not even validated if platform preconditions do not hold?

Thanks for the review. The intention is that the last four tests in TestPreconditions.java test precedence through platform-specific VM flags. As an example testApplyPlatformSVE will fail with Could not find VM flag "UseSVE" if we remove the applyIfPlatform precondition on x64.

Right, thanks for the clarification!

openjdk · 2023-09-29T12:17:44Z

@danielogh 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:

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

Reviewed-by: rcastanedalo, epeter, thartmann

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 214 new commits pushed to the master branch:

9718f49: 8317452: [JVMCI] Export symbols used by lightweight locking to JVMCI compilers.
0d4de8a: 8316971: Add Lint warning for restricted method calls
d4c904d: 8317294: Classloading throws exceptions over already pending exceptions
48f1a92: 8316679: C2 SuperWord: wrong result, load should not be moved before store if not comparable
0b0f8b5: 8219652: [aix] Tests failing with JNI attach problems.
8c0d026: 8315042: NPE in PKCS7.parseOldSignedData
f7deaf4: 8316778: test hprof lib: invalid array element type from JavaValueArray.elementSize
8ff10a0: 8317446: ProblemList gc/arguments/TestNewSizeFlags.java on macosx-aarch64 in Xcomp
1809b8c: 8317265: ListFormat::format specification could be made clearer regarding handling IllegalArgumentException.
cfabcbf: 8317121: vector_masked_load instruction is moved too early after JDK-8286941
... and 204 more: https://git.openjdk.org/jdk/compare/670b4567cf8229c9fd40c639a04dd1f1b7cfd551...master

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, @robcasloz, @eme64, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

eme64 · 2023-10-02T09:02:00Z

Thanks for doing this, @danielogh !
I thinks this patch is good. But please add the rules requested here #15938 (comment).
And make sure to file the follow-up RFE and link it to this issue here #15938 (comment).

Update: And I think you don't need to add more IR rules. You already have tests that use VM flags that only exist on certain platforms. Platform features need to be evaluated before VM flags, but not necessarily before CPU features. (just repeating what we discussed offline)

Update 2: I filed the follow-up RFE: JDK-8317341

eme64

Thanks for the work, looks good to me :)

TobiHartmann

Nice enhancement! Looks good to me.

danielogh · 2023-10-04T11:48:53Z

Thanks @eme64 and @TobiHartmann for reviews

danielogh · 2023-10-04T11:49:00Z

/integrate

openjdk · 2023-10-04T11:50:33Z

@danielogh
Your change (at version 2cfb95f) is now ready to be sponsored by a Committer.

eme64 · 2023-10-04T11:56:41Z

Thanks for all the work on this, we wanted this for a while!
/sponsor

openjdk · 2023-10-04T11:58:19Z

Going to push as commit a8549b6.
Since your change was applied there have been 214 commits pushed to the master branch:

9718f49: 8317452: [JVMCI] Export symbols used by lightweight locking to JVMCI compilers.
0d4de8a: 8316971: Add Lint warning for restricted method calls
d4c904d: 8317294: Classloading throws exceptions over already pending exceptions
48f1a92: 8316679: C2 SuperWord: wrong result, load should not be moved before store if not comparable
0b0f8b5: 8219652: [aix] Tests failing with JNI attach problems.
8c0d026: 8315042: NPE in PKCS7.parseOldSignedData
f7deaf4: 8316778: test hprof lib: invalid array element type from JavaValueArray.elementSize
8ff10a0: 8317446: ProblemList gc/arguments/TestNewSizeFlags.java on macosx-aarch64 in Xcomp
1809b8c: 8317265: ListFormat::format specification could be made clearer regarding handling IllegalArgumentException.
cfabcbf: 8317121: vector_masked_load instruction is moved too early after JDK-8286941
... and 204 more: https://git.openjdk.org/jdk/compare/670b4567cf8229c9fd40c639a04dd1f1b7cfd551...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-04T11:58:32Z

@eme64 @danielogh Pushed as commit a8549b6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

danielogh added 9 commits September 19, 2023 10:09

rebased

f009b8a

typo

89c0ee9

rebase

8083bac

tweak test

8f100f3

whitespace; tweak verified list using vm.simpleArch; put platform fea…

cbd7aa8

…tures before cpu features; lint new test; tweak tests.

Use Platform methods in checkPlatformFeature

5f6c65c

comment

49142d8

update README.md. Tweak test.

754f947

small tweak

e8d1546

openjdk bot added the rfr Pull request is ready for review label Sep 27, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 27, 2023

chhagedorn suggested changes Sep 27, 2023

View reviewed changes

robcasloz reviewed Sep 27, 2023

View reviewed changes

danielogh added 5 commits September 28, 2023 06:49

Add more platforms in IREncodingPrinter.java

bd0dfaf

platformFeature to platform. Lint docs. Add more tests.

a3bc95d

whitespace

944e844

whitespace

c9e151b

test method name

2cfb95f

robcasloz approved these changes Sep 29, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 29, 2023

eme64 approved these changes Oct 2, 2023

View reviewed changes

TobiHartmann approved these changes Oct 4, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 4, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 4, 2023

openjdk bot closed this Oct 4, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danielogh commented Sep 27, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 27, 2023

openjdk bot commented Sep 27, 2023

mlbridge bot commented Sep 27, 2023 •

edited

Loading

chhagedorn left a comment

chhagedorn Sep 27, 2023

chhagedorn Sep 27, 2023

chhagedorn Sep 27, 2023

chhagedorn Sep 27, 2023

chhagedorn Sep 27, 2023

robcasloz left a comment

robcasloz Sep 27, 2023

robcasloz Sep 27, 2023

robcasloz Sep 27, 2023

robcasloz Sep 27, 2023

robcasloz Sep 27, 2023

robcasloz Sep 27, 2023

robcasloz Sep 27, 2023

eme64 Oct 2, 2023

eme64 Oct 2, 2023

danielogh commented Sep 29, 2023

robcasloz commented Sep 29, 2023

danielogh commented Sep 29, 2023

robcasloz commented Sep 29, 2023

openjdk bot commented Sep 29, 2023 •

edited

Loading

eme64 commented Oct 2, 2023 •

edited

Loading

eme64 left a comment

TobiHartmann left a comment

danielogh commented Oct 4, 2023

danielogh commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

eme64 commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

		@@ -118,6 +118,11 @@ Sometimes, an `@IR` rule should only be applied if a certain CPU feature is pres

		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).

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

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

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

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

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

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

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

Conversation

danielogh commented Sep 27, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 27, 2023

openjdk bot commented Sep 27, 2023

mlbridge bot commented Sep 27, 2023 • edited Loading

Webrevs

chhagedorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robcasloz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielogh commented Sep 29, 2023

robcasloz commented Sep 29, 2023

danielogh commented Sep 29, 2023

robcasloz commented Sep 29, 2023

openjdk bot commented Sep 29, 2023 • edited Loading

eme64 commented Oct 2, 2023 • edited Loading

eme64 left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

danielogh commented Oct 4, 2023

danielogh commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

eme64 commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

danielogh commented Sep 27, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 27, 2023 •

edited

Loading

openjdk bot commented Sep 29, 2023 •

edited

Loading

eme64 commented Oct 2, 2023 •

edited

Loading