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

8252684: Move the AArch64 assember test under test/hotspot/gtest #1476

Closed
wants to merge 2 commits into from

Conversation

@nick-arm
Copy link
Member

@nick-arm nick-arm commented Nov 27, 2020

There was a question on the SVE review thread on build-dev [1] a few
months ago about why there is a Python script and test code under
src/hotspot/cpu/aarch64. The script generates code to check the
Assembler instruction encodings against those of the system assembler.
The test runs every time the debug VM is started.

AFAIK there's no precedent in the rest of Hotspot for having functional
tests that run on startup, and we have the existing gtest framework for
testing internal C++ modules. This patch (perhaps more of an RFC) moves
the assembler test under test/hotspot/gtest/aarch64.

The test will now run in tier1, including for release builds. The
downside is that debug builds won't catch assembler encoding errors
immediately on startup.

Tested by injecting an error in one of the instruction encodings and
verifying make test TEST="gtest" fails.

[1] https://mail.openjdk.java.net/pipermail/build-dev/2020-August/028048.html


Progress

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

Testing

Linux additional Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (8/8 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) (2/2 failed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) (1/9 failed) ✔️ (9/9 passed)

Failed test tasks

Issue

  • JDK-8252684: Move the AArch64 assember test under test/hotspot/gtest

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1476/head:pull/1476
$ git checkout pull/1476

There was a question on the SVE review thread on build-dev [1] a few
months ago about why there is a Python script and test code under
src/hotspot/cpu/aarch64. The script generates code to check the
Assembler instruction encodings against those of the system assembler.
The test runs every time the debug VM is started.

AFAIK there's no precedent in the rest of Hotspot for having functional
tests that run on startup, and we have the existing gtest framework for
testing internal C++ modules. This patch (perhaps more of an RFC) moves
the assembler test under test/hotspot/gtest/aarch64 out of the src/
directory.

The test will now run in tier1, including for release builds. The
downside is that debug builds won't catch assembler encoding errors
immediately on startup.

Tested by injecting an error in one of the instruction encodings and
verifying `make test TEST="gtest"` fails.

[1] https://mail.openjdk.java.net/pipermail/build-dev/2020-August/028048.html
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 27, 2020

👋 Welcome back ngasson! 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 label Nov 27, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 27, 2020

@nick-arm 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
Copy link

@mlbridge mlbridge bot commented Nov 27, 2020

Webrevs

@luhenry
Copy link
Member

@luhenry luhenry commented Nov 27, 2020

From having worked on the Windows and macOS port to AArch64, I concur that moving the test to the already dedicated gtest test suite makes more sense.

I now wonder if we could even get a step further and have the Python script generate the entirety of test/hotspot/gtest/aarch64/test_assembler_aarch64.cpp. It would be about adding just the copyright, the headesr #include, and some glue code. That would make it easier to regenerate the file instead of depending on a copy-paste.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Nov 27, 2020

I now wonder if we could even get a step further and have the Python script generate the entirety of test/hotspot/gtest/aarch64/test_assembler_aarch64.cpp. It would be about adding just the copyright, the headesr #include, and some glue code. That would make it easier to regenerate the file instead of depending on a copy-paste.

I suppose it doesn't need to generate the whole file: the Python script can just write its output to a file that gets #included into test_assembler_aarch64.cpp.

@theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Nov 27, 2020

I suppose it doesn't need to generate the whole file: the Python script can just write its output to a file that gets #included into test_assembler_aarch64.cpp.

Indeed. I did wonder about regenerating everything by running the Python script at test time, but reasoned that doing so would introduce a dependency on build tools ("as", for example) that I didn't want to have. But moving the output of aarch64_asmtest.py into a separate file is a good idea, if you like.

I'm minded to just approve this patch as it is now.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Nov 28, 2020

Indeed. I did wonder about regenerating everything by running the Python script at test time, but reasoned that doing so would introduce a dependency on build tools ("as", for example) that I didn't want to have. But moving the output of aarch64_asmtest.py into a separate file is a good idea, if you like.

I moved the output to a separate file asmtest.out.h with note on how to generate it.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 28, 2020

@nick-arm 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:

8252684: Move the AArch64 assember test under test/hotspot/gtest

Reviewed-by: aph

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:

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 label Nov 28, 2020
@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Nov 28, 2020

/integrate

@openjdk openjdk bot closed this Nov 28, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 28, 2020

@nick-arm Since your change was applied there have been 14 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit c93f0a0.

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

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Nov 28, 2020

This has broken all the regular x86 Windows builds! Please fix or backout immediately!

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Nov 29, 2020

IIUC given:

#ifdef AARCH64
#include "precompiled.hpp"

VS will ignore the ifdef as it appears before the precompiled header include.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Nov 29, 2020

This has broken all the regular x86 Windows builds! Please fix or backout immediately!

@dholmes-ora sorry I don't have access to a computer at the moment, would you mind reverting it for me? The added file is wrapped in #ifdef AARCH64 so I don't know why that affects windows x86.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Nov 29, 2020

The ifdef needs to come after the include of the precompiled header. I'm filing a bug. But it is Sunday for me too and I have very limited time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants