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

8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags #3829

Closed
wants to merge 1 commit into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented May 1, 2021

Hi all,

Could you please review this small and trivial patch that adds @requires vm.flagless to compiler/intrinsics/sha/cli tests that ignore VM flags?

This change makes sense since it will fix some test failures when testing with extra VM flags.

For example, the following three failures will be fixed when testing with UseAVX<2 on Intel CPUs.

compiler/intrinsics/sha/cli/TestUseSHA256IntrinsicsOptionOnUnsupportedCPU.java
compiler/intrinsics/sha/cli/TestUseSHAOptionOnUnsupportedCPU.java
compiler/intrinsics/sha/cli/TestUseSHA512IntrinsicsOptionOnUnsupportedCPU.java

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-8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags

Reviewers

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/3829
$ git pull https://git.openjdk.java.net/jdk pull/3829/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3829

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3829.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 1, 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.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented May 1, 2021

/label add hotspot-compiler
/cc hotspot-compiler

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2021

@DamonFool
The hotspot-compiler label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2021

@DamonFool The hotspot-compiler label was already applied.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 1, 2021

Webrevs

Loading

@iignatev
Copy link
Member

@iignatev iignatev commented May 1, 2021

Hi @DamonFool,

if these tests ignore external flags, how do they fail w/ -XX:UseAVX={0,1}?

Thanks,
-- Igor

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented May 1, 2021

if these tests ignore external flags, how do they fail w/ -XX:UseAVX={0,1}?

Well, yes and no.

These tests will first start a main test process, which won't ignore external flags.
Then the main test process will create new sub-test processes, which do ignore external flags.

For example, if we run TestUseSHA256IntrinsicsOptionOnUnsupportedCPU.java with UseAVX=1 on an Intel CPU, then

  1. The main test process will say SHA256_INSTRUCTION is not supported [1] since -XX:UseAVX=1, and then calls GenericTestCaseForUnsupportedX86CPU [2].
  2. Then a new sub-test process will be created in GenericTestCaseForUnsupportedX86CPU which expects UseSHA256Intrinsics to be false.
  3. But the sub-test process will see UseSHA256Intrinsics is actually true since it ignores external flags.
  4. Finally, the test fails.

So these tests do ignore external flags in all the sub-test processes.
And the tests may fail since the main test processes still accepts external flags, which is unexpected.

It would be better to mark them as vm.flagless to reduce testing noise.
Thnaks.

[1] https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/testlibrary/sha/predicate/IntrinsicPredicates.java#L85
[2] https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/intrinsics/sha/cli/TestUseSHA256IntrinsicsOptionOnUnsupportedCPU.java#L49

Loading

@iignatev
Copy link
Member

@iignatev iignatev commented May 1, 2021

Right, now I remember them :) they were actually in my first webrev for this.

I’m not clear on your decision to add the copyright line to TestUseSHA3IntrinsicsOptionOnUnsupportedCPU.java, but not other files.

— Igor

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented May 1, 2021

Right, now I remember them :) they were actually in my first webrev for this.

I’m not clear on your decision to add the copyright line to TestUseSHA3IntrinsicsOptionOnUnsupportedCPU.java, but not other files.

— Igor

This is because @vnkozlov taught me that we should add a new copyright line if the original line isn't Oracle, not just modify the copyright year.
Thanks.

Loading

Copy link
Member

@iignatev iignatev left a comment

IANAL. you should never change other companies' copyright. if you feel that your changes meet your criteria to add your copyright line, you should add it regardless of the existence/absence of Oracle copyright.

in any case, the patch looks good to me.

-- Igor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 2, 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:

8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags

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

  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • 3e667cc: 8265356: need code example for getting canonical constructor of a Record

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.

Loading

@openjdk openjdk bot added the ready label May 2, 2021
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented May 2, 2021

IANAL. you should never change other companies' copyright. if you feel that your changes meet your criteria to add your copyright line, you should add it regardless of the existence/absence of Oracle copyright.

in any case, the patch looks good to me.

-- Igor

Thanks @iignatev for your review.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented May 2, 2021

Thanks @vnkozlov .
/integrate

Loading

@openjdk openjdk bot closed this May 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 2, 2021

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

  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • 3e667cc: 8265356: need code example for getting canonical constructor of a Record

Your commit was automatically rebased without conflicts.

Pushed as commit 7e30130.

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

Loading

@DamonFool DamonFool deleted the JDK-8266401 branch May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants