Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263904: compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32 #3104

Closed
wants to merge 1 commit into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Mar 21, 2021

Hi all,

compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32.
The reason is that currently the bzhiq instruction is only generated on x86_64.
So it shouldn't be run on x86_32.

Thanks.
Best regards,
Jie


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263904: compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3104/head:pull/3104
$ git checkout pull/3104

To update a local copy of the PR:
$ git checkout pull/3104
$ git pull https://git.openjdk.java.net/jdk pull/3104/head

@DamonFool
Copy link
Member Author

/issue add JDK-8263904
/test
/label add hotspot-compiler
/cc hotspot-compiler

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 21, 2021

👋 Welcome back jiefu! 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 openjdk bot added the rfr Pull request is ready for review label Mar 21, 2021
@openjdk
Copy link

openjdk bot commented Mar 21, 2021

@DamonFool This issue is referenced in the PR title - it will now be updated.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 21, 2021
@openjdk
Copy link

openjdk bot commented Mar 21, 2021

@DamonFool
The hotspot-compiler label was successfully added.

@openjdk
Copy link

openjdk bot commented Mar 21, 2021

@DamonFool The hotspot-compiler label was already applied.

@mlbridge
Copy link

mlbridge bot commented Mar 21, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.

@openjdk
Copy link

openjdk bot commented Mar 22, 2021

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

8263904: compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32

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

  • f84b52b: 8263897: compiler/c2/aarch64/TestVolatilesSerial.java failed with "java.lang.RuntimeException: Wrong method"
  • f08bf4b: 8263891: Changes for 8076985 missed the fix.
  • b2df513: 8261785: Calling "main" method in anonymous nested class crashes the JVM
  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • 6c2220e: 8263861: Shenandoah: Remove unused member in ShenandoahGCStateResetter
  • 5262d95: 8263855: Use the blessed modifier order in java.management/naming
  • 6f1bcb0: 8263593: Fix multiple typos in hsdis README
  • a9d2267: 8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*)
  • ... and 4 more: https://git.openjdk.java.net/jdk/compare/118a49fc9699590fb5c935729da64dac5e61f26d...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 22, 2021
Copy link
Contributor

@mgkwill mgkwill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me.

@iwanowww
Copy link

The test already does some platform sensing itself (in BmiIntrinsicBase::test()), so mixing it with @requires doesn't look pretty.

What about something like the following?

diff --git a/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BmiIntrinsicBase.java b/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BmiIntrinsicBase.java
index 47adc9e98fb..be605108caa 100644
--- a/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BmiIntrinsicBase.java
+++ b/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BmiIntrinsicBase.java
@@ -63,6 +63,10 @@ public class BmiIntrinsicBase extends CompilerWhiteBoxTest {
             return;
         }
 
+        if ((bmiTestCase instanceof BmiTestCase_x64Only) && !Platform.isX64()) {
+            System.out.println("Unsupported platform, test SKIPPED");
+        }
+
         if (!Platform.isServer()) {
             throw new Error("TESTBUG: Not server VM");
         }
@@ -227,4 +231,10 @@ public class BmiIntrinsicBase extends CompilerWhiteBoxTest {
             return cnt;
         }
     }
+
+    abstract static class BmiTestCase_x64Only extends BmiTestCase_x64 {
+        protected BmiTestCase_x64Only(Method method) {
+            super(method);
+        }
+    }
 }
diff --git a/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java b/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java
index 8a55a236f51..531e9077dbf 100644
--- a/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java
+++ b/test/hotspot/jtreg/compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java
@@ -41,7 +41,7 @@ import compiler.intrinsics.bmi.TestBzhiI2L;
 
 import java.lang.reflect.Method;
 
-public class BzhiTestI2L extends BmiIntrinsicBase.BmiTestCase_x64 {
+public class BzhiTestI2L extends BmiIntrinsicBase.BmiTestCase_x64Only {
 
     protected BzhiTestI2L(Method method) {
         super(method);

@mgkwill
Copy link
Contributor

mgkwill commented Mar 22, 2021

Hi @iwanowww, currently a child of BmiIntrinsicBase.BmiTestCase_x64 has both, instrMask, instrPattern and instrMask_x64, instrPattern_x64. Theoretically you could test both x86_32 and x86_64 using the same test code if Platform.isX86() or Platform.isX64() are used to differentiate. See #3128 as a possible fix to BmiIntrinsicBase.BmiTestCase_x64 question and extra platform check to insure BzhiTestI2L does not run on x86 for now.

I don't mind overlapping @requires vm.simpleArch == "x64", what's done here, with the changes I've proposed.

@DamonFool
Copy link
Member Author

Thanks @vnkozlov , @mgkwill and @iwanowww for your review and comments.
/integrate

@openjdk openjdk bot closed this Mar 22, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 22, 2021
@DamonFool DamonFool deleted the JDK-8263904 branch March 22, 2021 23:14
@openjdk
Copy link

openjdk bot commented Mar 22, 2021

@DamonFool Since your change was applied there have been 15 commits pushed to the master branch:

  • f62b100: 8263895: Test nsk/jvmti/GetThreadGroupChildren/getthrdgrpchld001/getthrdgrpchld001.cpp uses incorrect indices
  • f84b52b: 8263897: compiler/c2/aarch64/TestVolatilesSerial.java failed with "java.lang.RuntimeException: Wrong method"
  • f08bf4b: 8263891: Changes for 8076985 missed the fix.
  • b2df513: 8261785: Calling "main" method in anonymous nested class crashes the JVM
  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • 6c2220e: 8263861: Shenandoah: Remove unused member in ShenandoahGCStateResetter
  • 5262d95: 8263855: Use the blessed modifier order in java.management/naming
  • 6f1bcb0: 8263593: Fix multiple typos in hsdis README
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/118a49fc9699590fb5c935729da64dac5e61f26d...master

Your commit was automatically rebased without conflicts.

Pushed as commit a5e7a89.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants