8302681: [IR Framework] Only allow cpuFeatures from a verified list by eme64 · Pull Request #12669 · openjdk/jdk

eme64 · 2023-02-20T15:11:21Z

Wrongly typed cpuFeatures can lead to an IR rule being ignored.
Example: #12601 JDK-8302668

We should have a list of verified cpuFeatures, and assert if one is used that is not in that list. That way, typos and bad copies can be avoided, and we make sure the tests run when intended.

I verified the intel cpuFeatures by hand on my laptop. And for asimd and sve I asked @tobiasholenstein and @nick-arm .

sve1 seems to have been a typo, and I am changing it to sve.

Note: cpu-features can be displayed like this: ./java -Xlog:os+cpu --version

On my machine, I get:

cmov, fxsr, ht, mmx, 3dnowpref, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, lzcnt, tsc, tscinvbit, avx, avx2, aes, erms, clmul, bmi1, bmi2, adx, avx512f, avx512dq, avx512cd, avx512bw, avx512vl, sha, fma, vzeroupper, avx512_vpopcntdq, avx512_vpclmulqdq, avx512_vaes, avx512_vnni, clflush, clflushopt, clwb, avx512_vbmi2, avx512_vbmi, rdtscp, rdpid, fsrm, gfni, avx512_bitalg, f16c, pku, ospke, cet_ibt, cet_ss, avx512_ifma

@nick-arm got this on one of his machines:

fp, asimd, evtstrm, aes, pmull, sha1, sha256, crc32, lse, dcpop, sha3, sha512, sve, paca

@tobiasholenstein got this:

fp, asimd, aes, pmull, sha1, sha256, crc32, lse, sha3, sha512

I had to adapt a test that used a non-existent cpu feature. I replaced it with an impossible combination.

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-8302681: [IR Framework] Only allow cpuFeatures from a verified list

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 340741eb
Pengfei Li (@pfustc - Committer) ⚠️ Review applies to 340741eb
Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to adaed88f
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12669

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

Using diff file

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

bridgekeeper · 2023-02-20T15:13:12Z

👋 Welcome back epeter! 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-02-20T15:15:45Z

@eme64 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.

eme64 · 2023-02-20T15:41:44Z

@fg1417 Could you tell me if sve and sve1 are actually cpu-featues that any hardware ever has? Especially sve1 seems suspicious to me.

Update sve is a real cpu-feature, and sve1 seems to have been a typo.

mlbridge · 2023-02-20T16:47:09Z

Webrevs

TobiHartmann

[TODO: some failures because of sse3. That may be another issue I need to fix.]

sse3 is missing from your list, right?

Should we simply use the CPU features detected by the VM? For example:

jdk/src/hotspot/cpu/x86/vm_version_x86.hpp

Lines 322 to 327 in f12710e

    
           #define CPU_FEATURE_FLAGS(decl) \ 
        
               decl(CX8,               "cx8",               0)  /*  next bits are from cpuid 1 (EDX) */ \ 
        
               decl(CMOV,              "cmov",              1)  \ 
        
               decl(FXSR,              "fxsr",              2)  \ 
        
               decl(HT,                "ht",                3)  \ 
        
                                                                \

jdk/src/hotspot/cpu/aarch64/vm_version_aarch64.hpp

Lines 105 to 110 in f12710e

    
           #define CPU_FEATURE_FLAGS(decl)               \ 
        
               decl(FP,            fp,            0)     \ 
        
               decl(ASIMD,         asimd,         1)     \ 
        
               decl(EVTSTRM,       evtstrm,       2)     \ 
        
               decl(AES,           aes,           3)     \ 
        
               decl(PMULL,         pmull,         4)     \

This test does that too:

jdk/test/lib-test/jdk/test/whitebox/CPUInfoTest.java

Lines 53 to 68 in f12710e

    
           wellKnownCPUFeatures = Set.of( 
        
                   "cx8",          "cmov",             "fxsr",              "ht", 
        
                   "mmx",          "3dnowpref",        "sse",               "sse2", 
        
                   "sse3",         "ssse3",            "sse4a",             "sse4.1", 
        
                   "sse4.2",       "popcnt",           "lzcnt",             "tsc", 
        
                   "tscinvbit",    "tscinv",           "avx",               "avx2", 
        
                   "aes",          "erms",             "clmul",             "bmi1", 
        
                   "bmi2",         "rtm",              "adx",               "avx512f", 
        
                   "avx512dq",     "avx512pf",         "avx512er",          "avx512cd", 
        
                   "avx512bw",     "avx512vl",         "sha",               "fma", 
        
                   "vzeroupper",   "avx512_vpopcntdq", "avx512_vpclmulqdq", "avx512_vaes", 
        
                   "avx512_vnni",  "clflush",          "clflushopt",        "clwb", 
        
                   "avx512_vbmi2", "avx512_vbmi",      "rdtscp",            "rdpid", 
        
                   "hv",           "fsrm",             "avx512_bitalg",     "gfni", 
        
                   "f16c",         "pku",              "ospke",             "cet_ibt", 
        
                   "cet_ss",       "avx512_ifma"

eme64 · 2023-02-21T11:56:49Z

@TobiHartmann I was actually missing a few cpu features. And a test needed to be corrected. I am running the tests again now.

I would like to keep the list small, and separate from other lists. It has to be clear that this list is verified, and if one adds something one has to be very sure that it is not a typo or non-existent.

If people need an additional feature, it can easily be verified and added.

TobiHartmann

I would like to keep the list small, and separate from other lists. It has to be clear that this list is verified, and if one adds something one has to be very sure that it is not a typo or non-existent.

Okay, fine with me.

TestPreconditions.java needs a copyright update.

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

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

TobiHartmann · 2023-02-21T12:23:09Z

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

+        // on this list, we will ignore tests and never execute them.
+        List<String> verifiedCPUFeatures = new ArrayList<String>( Arrays.asList(
+            // General
+            "sha3",


Isn't sha3 AArch64 specific?

TobiHartmann · 2023-02-21T12:24:39Z

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

+        List<String> verifiedCPUFeatures = new ArrayList<String>( Arrays.asList(
+            // General
+            "sha3",
+            "fma",


I would put this under x86 or something.

Please add

// Consult CPU_FEATURE_FLAGS in corresponding vm_version_.hpp file to find correct cpu feature's name.

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

TobiHartmann

Thanks for making these changes. Looks good to me.

openjdk · 2023-02-22T10:09:10Z

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

8302681: [IR Framework] Only allow cpuFeatures from a verified list

Reviewed-by: thartmann, pli, chagedorn, kvn

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

6397cb6: 8301200: Don't scale timeout stress with timeout factor
71dd7ea: 8303085: Runtime problem list cleanup
4b6acad: 8302883: JFR: Improve periodic events
a2471b3: 8303125: ProblemList vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java
58ca711: 8303078: Reduce allocations when pretty printing JCTree during compilation
2cf8b86: 8303071: Memory leaks in libjdwp
f113b04: 8302927: Unproblemlist Fuzz.java from ProblemList-zgc.txt again
1bab93b: 8303073: (bf) Temporarily reinstate private DirectByteBuffer(long, int) constructor
221f346: 8303047: avoid NULL after 8301661
23e9d9d: 8302811: NMT.random_reallocs_vm fails if NMT is off
... and 65 more: https://git.openjdk.org/jdk/compare/71cf7c4409025c87ac786a54171f00de69fe5317...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

pfustc

Thank you for fixing this. LGTM

chhagedorn

Otherwise, looks good to me, too!

chhagedorn · 2023-02-23T10:36:50Z

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

+        // Please verify new CPU features before adding them. If we allow non-existent features
+        // on this list, we will ignore tests and never execute them. Consult CPU_FEATURE_FLAGS
+        // in corresponding vm_version_.hpp file to find correct cpu feature's name.
+        List<String> verifiedCPUFeatures = new ArrayList<String>( Arrays.asList(


I suggest to convert this local variable to a private static final field.

vnkozlov

Good.

eme64 · 2023-02-27T07:11:22Z

@pfustc @chhagedorn @TobiHartmann @vnkozlov thanks for the reviews and suggestions!
/integrate

openjdk · 2023-02-27T07:12:25Z

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

a43931b: 8303102: jcmd: ManagementAgent.status truncates the text longer than O_BUFLEN
2fb1e3b: 8302268: Prefer ArrayList to LinkedList in XEmbeddedFramePeer
3a9f491: 8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response
1dbd18a: 8302120: Prefer ArrayList to LinkedList in AggregatePainter
2e78d83: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor
17e3769: 8302871: Speed up StringLatin1.regionMatchesCI
b4ea807: 8288912: vmTestbase/nsk/stress/strace/strace002.java fails with Unexpected method name: currentCarrierThread
ccf3340: 8303083: (bf) Remove private DirectByteBuffer(long, int) constructor before JDK 21 GA
83d77b1: 8303072: Memory leak in exeNullCallerTest.cpp
7d8b8ba: 8303131: pandoc.exe mangles all processed html files
... and 81 more: https://git.openjdk.org/jdk/compare/71cf7c4409025c87ac786a54171f00de69fe5317...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-27T07:12:36Z

@eme64 Pushed as commit 1794f49.

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

8302681: [IR Framework] Only allow cpuFeatures from a verified list

629f8ff

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 20, 2023

sve1 was a typo, fix that

f0e920c

eme64 marked this pull request as ready for review February 20, 2023 16:41

openjdk bot added the rfr Pull request is ready for review label Feb 20, 2023

TobiHartmann suggested changes Feb 21, 2023

View reviewed changes

eme64 added 2 commits February 21, 2023 12:44

Added a few more cpu features

1196e97

modified test that used non-existent cpu feature

8329590

add comment

3d836fb

TobiHartmann suggested changes Feb 21, 2023

View reviewed changes

eme64 and others added 3 commits February 21, 2023 13:26

copyright

92f6e96

Apply suggestions from code review from @TobiHartmann

12eae7e

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

more fixes after Tobias review

340741e

eme64 requested a review from TobiHartmann February 22, 2023 08:01

TobiHartmann approved these changes Feb 22, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 22, 2023

pfustc approved these changes Feb 23, 2023

View reviewed changes

Added Vladimir K's request comment

adaed88

chhagedorn approved these changes Feb 23, 2023

View reviewed changes

Christian's review suggestion: private static final

64d3612

vnkozlov approved these changes Feb 23, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 27, 2023

openjdk bot closed this Feb 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 27, 2023

	#define CPU_FEATURE_FLAGS(decl) \
	decl(CX8, "cx8", 0) /* next bits are from cpuid 1 (EDX) */ \
	decl(CMOV, "cmov", 1) \
	decl(FXSR, "fxsr", 2) \
	decl(HT, "ht", 3) \
	\

	#define CPU_FEATURE_FLAGS(decl) \
	decl(FP, fp, 0) \
	decl(ASIMD, asimd, 1) \
	decl(EVTSTRM, evtstrm, 2) \
	decl(AES, aes, 3) \
	decl(PMULL, pmull, 4) \

	wellKnownCPUFeatures = Set.of(
	"cx8", "cmov", "fxsr", "ht",
	"mmx", "3dnowpref", "sse", "sse2",
	"sse3", "ssse3", "sse4a", "sse4.1",
	"sse4.2", "popcnt", "lzcnt", "tsc",
	"tscinvbit", "tscinv", "avx", "avx2",
	"aes", "erms", "clmul", "bmi1",
	"bmi2", "rtm", "adx", "avx512f",
	"avx512dq", "avx512pf", "avx512er", "avx512cd",
	"avx512bw", "avx512vl", "sha", "fma",
	"vzeroupper", "avx512_vpopcntdq", "avx512_vpclmulqdq", "avx512_vaes",
	"avx512_vnni", "clflush", "clflushopt", "clwb",
	"avx512_vbmi2", "avx512_vbmi", "rdtscp", "rdpid",
	"hv", "fsrm", "avx512_bitalg", "gfni",
	"f16c", "pku", "ospke", "cet_ibt",
	"cet_ss", "avx512_ifma"

Conversation

eme64 commented Feb 20, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 20, 2023

Uh oh!

openjdk bot commented Feb 20, 2023

Uh oh!

eme64 commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

eme64 commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TobiHartmann Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

vnkozlov Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfustc left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

eme64 commented Feb 27, 2023

Uh oh!

openjdk bot commented Feb 27, 2023

Uh oh!

openjdk bot commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

eme64 commented Feb 20, 2023 •

edited by openjdk bot

Loading

eme64 commented Feb 20, 2023 •

edited

Loading

mlbridge bot commented Feb 20, 2023 •

edited

Loading

eme64 commented Feb 21, 2023 •

edited

Loading

openjdk bot commented Feb 22, 2023 •

edited

Loading