8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests #15011

pfustc · 2023-07-25T08:35:11Z

This patch removes @require vm.flagless annotations from HotSpot jtreg tests in compiler/vectorization/runner. All jtreg cases in this folder are invoked by test driver VectorizationTestRunner.java which checks both correctness and vectorizability (IR) for each test method. We added flagless requirement before because extra flags may mess with compiler control in the test driver for correctness check. But flagless has a side effect that it makes tests with extra flags skipped. So we propose to get rid of it now.

To adapt the removal of @require vm.flagless, a few checks are added in the test driver to skip the correctness check if extra flags make the compiler control not work. This patch also moves previously hard-coded flag -XX:-OptimizeFill in the test driver to conditions in IR checks.

Tested various of compiler control related VM flags on x86 and AArch64.

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-8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests (Enhancement - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)
Emanuel Peter (@eme64 - Reviewer)
Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15011

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

Using diff file

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

Webrev

Link to Webrev Comment

…ation tests This patch removes `@require vm.flagless` annotations from HotSpot jtreg tests in `compiler/vectorization/runner`. All jtreg cases in this folder are invoked by test driver `VectorizationTestRunner.java` which checks both correctness and vectorizability (IR) for each test method. We added flagless requirement before because extra flags may mess with compiler control in the test driver for correctness check. But `flagless` has a side effect that it makes tests with extra flags skipped. So we propose to get rid of it now. To adapt the removal of `@require vm.flagless`, a few checks are added in the test driver to skip the correctness check if extra flags make the compiler control not work. This patch also moves previously hard-coded flag `-XX:-OptimizeFill` in the test driver to conditions in IR checks. Tested various of compiler control related VM flags on x86 and AArch64.

bridgekeeper · 2023-07-25T08:36:13Z

👋 Welcome back pli! 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-07-25T08:37:19Z

@pfustc 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-07-25T08:40:58Z

Webrevs

pfustc · 2023-07-25T08:49:10Z

Hi @eme64 @vnkozlov, This removes flagless from jtreg vectorization tests as you previously suggested. Could you help take a look?

vnkozlov · 2023-07-26T18:18:16Z

test/hotspot/jtreg/compiler/vectorization/runner/VectorizationTestRunner.java

+        boolean use_intp = WB.getBooleanVMFlag("UseInterpreter");
+        boolean use_comp = WB.getBooleanVMFlag("UseCompiler");
+        boolean bg_comp = WB.getBooleanVMFlag("BackgroundCompilation");
+        if (use_intp && use_comp && bg_comp) {


Why you check BackgroundCompilation?
For -Xcomp you need only check UseInterpreter.
Xor -Xint you need only check UseCompiler.

For -Xbatch we need check BackgroundCompilation. As we lock the compilation before running the test method in the interpreter to get reference result, the VM will hang if both background compilation is disabled and the compilation is locked by WhiteBox.

I just experimented an alternative approach that running reference result with C1 execution instead of interpretation. In this way we can run with background compilation off. What do you think of this new approach?

vnkozlov · 2023-07-26T18:29:06Z

test/hotspot/jtreg/compiler/vectorization/runner/VectorizationTestRunner.java

-                fail("Method is not compiled after " + COMP_THRES_SECONDS + "s.");
+                // C2 compilation may timeout with extra compiler control options.
+                // We skip the correctness check in this case.
+                System.out.println("WARNING: Correctness check is skipped because C2 compilation times out");
+                return;


Why not run with BackgroundCompilation OFF? The code will return from WHITE_BOX.enqueueMethodForCompilation() only after finishing compiling or compiler skip.
Compilation speed is very different on different platform. Using time for check is not reliable.

Why not run with BackgroundCompilation OFF?

I just explained the reason in my another reply above. Using time for check is to avoid jtreg timeout with extra VM options like -XX:CompileThreshold=100 -XX:-TieredCompilation. But I think this can also be avoided if we get reference result from C1 execution. What do you think of the new approach?

TobiHartmann · 2023-07-27T07:23:48Z

LoopArrayIndexComputeTest.java fails on x64 with -XX:UseAVX=0 -XX:UseSSE=3 and with -XX:UseAVX=0 -XX:UseSSE=2:

Failed IR Rules (1) of Methods (1)
----------------------------------
1) Method "public byte[] compiler.vectorization.runner.LoopArrayIndexComputeTest.byteArrayWithDependencePos()" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(applyIfCPUFeatureAnd={}, phase={DEFAULT}, applyIfCPUFeatureOr={"asimd", "true", "sse2", "true"}, applyIf={"AlignVector", "false"}, applyIfCPUFeature={}, counts={"_#STORE_VECTOR#_", ">0"}, failOn={}, applyIfAnd={}, applyIfOr={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(StoreVector.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 > 0 [given]
           - No nodes matched!

pfustc · 2023-07-31T07:52:23Z

Hi @TobiHartmann ,

LoopArrayIndexComputeTest.java fails on x64 with -XX:UseAVX=0 -XX:UseSSE=3 and with -XX:UseAVX=0 -XX:UseSSE=2:

I just tried this on multiple x86 machines we have but didn't reproduce the failure. Could you share more info (cpu features, etc.) of your test machine so I can find why the test method is not vectorized.

vnkozlov · 2023-07-31T19:55:19Z

Hi @TobiHartmann ,

LoopArrayIndexComputeTest.java fails on x64 with -XX:UseAVX=0 -XX:UseSSE=3 and with -XX:UseAVX=0 -XX:UseSSE=2:

I just tried this on multiple x86 machines we have but didn't reproduce the failure. Could you share more info (cpu features, etc.) of your test machine so I can find why the test method is not vectorized.

The test has Byte multiply operation but it checks only presence of SSE2:
@IR(applyIfCPUFeatureOr = {"asimd", "true", "sse2", "true"}, ... res[i] *= bytes[i + 3];
But we don't support it with SSE < 4 - see Matcher::match_rule_supported().

vm.flagless prevented running it before with such flags combination.

I am not sure how your testing passed.

pfustc · 2023-08-02T08:53:03Z

Hi @vnkozlov , I re-worked the correctness check in my latest commit. This time we get reference results from C1 execution so we can allow "-Xbatch" and remove the time-based check now. But we have to add another check of tiered compilation for C1 execution. The failed case is also fixed in my latest commit. Could you help re-review?

vnkozlov · 2023-08-03T19:27:11Z

Thank you for addressing the test issue.

About your change to allow -Xbatch. Let me clarify, if you exclude -Xcomp mode (which I agree with) by checking UseInterpreter flag for true, then a method could be always executed in Interpeter to get reference result (even with -XX:CompileThreshold=100) by calling method once first (we do that in other tests).
You don't need to limit test to Tiered mode when Interpreter is available and original test code should work.

For -Xbatch we need check BackgroundCompilation. As we lock the compilation before running the test method in the interpreter to get reference result, the VM will hang if both background compilation is disabled and the compilation is locked by WhiteBox.

You don't need to call WB.lockCompilation() if you exclude -Xcomp mode. There will be no compilation requests for called method when you call the method first time because compilation threshold will not be reached - it is guarantee that method will be executed in Interpreter. And you have the assert to verify that.

Note, this work with and without BackgroundCompilation enabled. If you called a method once it will be executed in Interpreter if -Xcomp is excluded.

pfustc · 2023-08-04T06:31:46Z

Hi @vnkozlov ,

Thanks for your reply. But it still has problems.

About your change to allow -Xbatch. Let me clarify, if you exclude -Xcomp mode (which I agree with) by checking UseInterpreter flag for true, then a method could be always executed in Interpeter to get reference result (even with -XX:CompileThreshold=100) by calling method once first (we do that in other tests).

You don't need to call WB.lockCompilation() if you exclude -Xcomp mode. There will be no compilation requests for called method when you call the method first time because compilation threshold will not be reached - it is guarantee that method will be executed in Interpreter. And you have the assert to verify that.

These tests are a bit different because we test loops. If the loop iteration count reaches some threshold, the loop will be OSR compiled even test method is called only once. I just did an experiment according to your suggestion. After removing WB.lockCompilation() and updating loop iteration count to 100,000, I got assertion failure that tells me the test method is NOT running in interpreter.

STDERR:
java.lang.AssertionError
	at compiler.vectorization.runner.VectorizationTestRunner.runTestOnMethod(VectorizationTestRunner.java:131)
	at compiler.vectorization.runner.VectorizationTestRunner.run(VectorizationTestRunner.java:73)
	at compiler.vectorization.runner.VectorizationTestRunner.main(VectorizationTestRunner.java:215)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
	at java.base/java.lang.Thread.run(Thread.java:1570)

A solution to this may be adding one more check of CICompileOSR is OFF if we still want to use interpreted execution for the reference result.

Now the question is, which verification approach do you think is better? "C2 vs. interpreted" or "C2 vs. C1"?

vnkozlov · 2023-08-04T20:10:19Z

A solution to this may be adding one more check of CICompileOSR is OFF if we still want to use interpreted execution for the reference result.

I would suggest to use WB.setBooleanVMFlag("CICompileOSR", false);. But it is debug flag which can be set only in debug VM. There are may be other product flags you can temporary set to avoid compilation without locking.

Now the question is, which verification approach do you think is better? "C2 vs. interpreted" or "C2 vs. C1"?

We usually use Interpreter as gold standard.

pfustc · 2023-08-07T09:53:28Z

Hi @vnkozlov ,

Thanks for your comments. I have reverted the patch to my 1st commit and re-addressed your comments.

I would suggest to use WB.setBooleanVMFlag("CICompileOSR", false);. But it is debug flag which can be set only in debug VM. There are may be other product flags you can temporary set to avoid compilation without locking.

In my new commit, I choose to set and restore UseCompiler before and after the interpreter run. I have re-tested various of compiler control options and no jtreg timeout is seen now. Please let me know if this looks good to you.

vnkozlov

This looks good.

openjdk · 2023-08-07T16:00:46Z

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

8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests

Reviewed-by: kvn, thartmann, epeter, chagedorn

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

11da15d: 8269957: facilitate alternate impls of NameTable and Name
725ec0c: 8315020: The macro definition for LoongArch64 zero build is not accurate.
1c3177e: 8315029: [BACKOUT] Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))
dd23f7d: 8315039: Parallel: Remove unimplemented PSYoungGen::oop_iterate
5c4f1dc: 8314513: [IR Framework] Some internal IR Framework tests are failing after JDK-8310308 on PPC and Cascade Lake
cf2d33c: 8299658: C1 compilation crashes in LinearScan::resolve_exception_edge
1664e79: 8311792: java/net/httpclient/ResponsePublisher.java fails intermittently with AssertionError: Found some outstanding operations
0901d75: 8314762: Make {@Incubating} conventional

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

eme64 · 2023-08-08T17:44:46Z

@vnkozlov @TobiHartmann we should re-run testing from our side.

@pfustc Why do you only test correctness (compare results) in some conditions? Is there not a risk that we miss doing it in some cases we should do it, just because we get the conditions slightly wrong?

Just FYI: we should integrate this whole correctness of results testing into the IR framework. I filed JDK-8310533. That would make it easier to use for new tests. It could also be used for any test, not just the ones located in test/hotspot/jtreg/compiler/vectorization.

pfustc · 2023-08-09T09:15:28Z

Hi @eme64 ,

Thanks for looking at this.

@pfustc Why do you only test correctness (compare results) in some conditions? Is there not a risk that we miss doing it in some cases we should do it, just because we get the conditions slightly wrong?

Yes, you are right! These conditions are added before to avoid jtreg hanging when compilation is locked. But now I can remove them because the lock is removed. In my latest commit, I have removed the conditions and some useless imports.

Just FYI: we should integrate this whole correctness of results testing into the IR framework. I filed JDK-8310533. That would make it easier to use for new tests. It could also be used for any test, not just the ones located in test/hotspot/jtreg/compiler/vectorization.

I have noticed this JBS before. The reason I didn't added correctness check into the IR framework is that I implemented this kind of check before the IR framework exists. (We have used it internally for a few years.) But anyway, it is a good proposal and I'm willing to help if needed.

eme64 · 2023-08-10T10:55:54Z

@pfustc This looks good to me, thanks for making these changes. It will really increase the coverage.
Maybe @vnkozlov should quickly look at it again if he still agrees.
@TobiHartmann is running the testing again, just in case the dropped conditions change something.
I will give you my approval after those tests are passing.

vnkozlov

I am fine with update.

TobiHartmann · 2023-08-11T06:37:32Z

All tests passed (@eme64).

eme64

Thanks @TobiHartmann .

-> approved

eme64 · 2023-08-15T12:57:10Z

@pfustc Since you are working on IR tests here, it would be good if you first merge the changes from JDK-8310308, and test the IR rules again.

pfustc · 2023-08-28T08:47:49Z

Thanks @vnkozlov @TobiHartmann @eme64 for your review. Patch is currently merged with latest master and re-tested. I will integrate this if there's no further comment.

chhagedorn · 2023-08-28T11:06:49Z

I will also give this another spin in our testing.

chhagedorn

Testing looked good!

pfustc · 2023-08-29T01:37:44Z

/integrate

openjdk · 2023-08-29T01:39:57Z

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

e5ea9aa: 8312570: [TESTBUG] Jtreg compiler/loopopts/superword/TestDependencyOffsets.java fails on 512-bit SVE
1cb2cc6: 8308464: Shared array class should not always be loaded in boot loader
69d1feb: 8315060: Out of tree incremental build fails with ccache
8e2a533: 8315137: Add explicit override RecordComponentElement.asType()
b4b2fec: 8311081: KeytoolReaderP12Test.java fail on localized Windows platform
31e2681: 8315071: Modify TrayIconScalingTest.java, PrintLatinCJKTest.java to use new PassFailJFrame's builder pattern usage
21916f3: 8139208: [macosx] Issue with setExtendedState of JFrame
99ea8bf: 8315062: [GHA] get-bootjdk action should return the abolute path
acb24bf: 8315116: fix minor issue in copyright header introduced by JDK-8269957 that is breaking the build
11da15d: 8269957: facilitate alternate impls of NameTable and Name
... and 7 more: https://git.openjdk.org/jdk/compare/12de9b0225363377e9a76729b11698221d4f29f2...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-29T01:40:13Z

@pfustc Pushed as commit a03954e.

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

openjdk bot added the rfr Pull request is ready for review label Jul 25, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 25, 2023

vnkozlov reviewed Jul 26, 2023

View reviewed changes

Re-work correctness check to allow "-Xbatch"

ac50968

pfustc requested a review from vnkozlov August 2, 2023 08:53

Revert to the 1st commit and re-address comments

5bb6700

vnkozlov approved these changes Aug 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 7, 2023

Remove useless conditions and imports

00d48cc

vnkozlov approved these changes Aug 10, 2023

View reviewed changes

TobiHartmann approved these changes Aug 11, 2023

View reviewed changes

eme64 approved these changes Aug 11, 2023

View reviewed changes

Merge branch 'master' into flagless

42de3da

chhagedorn approved these changes Aug 28, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 29, 2023

openjdk bot closed this Aug 29, 2023

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

pfustc deleted the flagless branch August 29, 2023 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests #15011

8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests #15011

pfustc commented Jul 25, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 25, 2023

openjdk bot commented Jul 25, 2023

mlbridge bot commented Jul 25, 2023 •

edited

Loading

pfustc commented Jul 25, 2023

vnkozlov Jul 26, 2023

pfustc Jul 31, 2023

vnkozlov Jul 26, 2023

pfustc Jul 31, 2023

TobiHartmann commented Jul 27, 2023

pfustc commented Jul 31, 2023

vnkozlov commented Jul 31, 2023

pfustc commented Aug 2, 2023

vnkozlov commented Aug 3, 2023

pfustc commented Aug 4, 2023

vnkozlov commented Aug 4, 2023

pfustc commented Aug 7, 2023

vnkozlov left a comment

openjdk bot commented Aug 7, 2023 •

edited

Loading

eme64 commented Aug 8, 2023

pfustc commented Aug 9, 2023

eme64 commented Aug 10, 2023

vnkozlov left a comment

TobiHartmann commented Aug 11, 2023

eme64 left a comment

eme64 commented Aug 15, 2023

pfustc commented Aug 28, 2023

chhagedorn commented Aug 28, 2023

chhagedorn left a comment

pfustc commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests #15011

8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests #15011

Conversation

pfustc commented Jul 25, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 25, 2023

openjdk bot commented Jul 25, 2023

mlbridge bot commented Jul 25, 2023 • edited Loading

Webrevs

pfustc commented Jul 25, 2023

vnkozlov Jul 26, 2023

Choose a reason for hiding this comment

pfustc Jul 31, 2023

Choose a reason for hiding this comment

vnkozlov Jul 26, 2023

Choose a reason for hiding this comment

pfustc Jul 31, 2023

Choose a reason for hiding this comment

TobiHartmann commented Jul 27, 2023

pfustc commented Jul 31, 2023

vnkozlov commented Jul 31, 2023

pfustc commented Aug 2, 2023

vnkozlov commented Aug 3, 2023

pfustc commented Aug 4, 2023

vnkozlov commented Aug 4, 2023

pfustc commented Aug 7, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 7, 2023 • edited Loading

eme64 commented Aug 8, 2023

pfustc commented Aug 9, 2023

eme64 commented Aug 10, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

TobiHartmann commented Aug 11, 2023

eme64 left a comment

Choose a reason for hiding this comment

eme64 commented Aug 15, 2023

pfustc commented Aug 28, 2023

chhagedorn commented Aug 28, 2023

chhagedorn left a comment

Choose a reason for hiding this comment

pfustc commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

pfustc commented Jul 25, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 25, 2023 •

edited

Loading

openjdk bot commented Aug 7, 2023 •

edited

Loading