-
Notifications
You must be signed in to change notification settings - Fork 41
8301769: Generational ZGC: Indirect access barriers are never elided #13
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
8301769: Generational ZGC: Indirect access barriers are never elided #13
Conversation
|
👋 Welcome back rcastanedalo! A progress list of the required criteria for merging this PR into |
fisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Nice!
|
@robcasloz 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: 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 124 new commits pushed to the
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 |
|
Unfortunately, we can't enable the test for PPC64, now. Parts of the optimization and the test don't work out of the box. |
Thanks for reviewing, Erik! |
Thanks for trying out the tests @TheRealMDoerr. Would you mind sharing their output here? There are two types of test cases: performance tests (unnecessary barriers are elided) and correctness tests (barriers that might be needed are preserved). While the former might fail on different platforms due to limitations in the analysis, I would like to check that none of the latter fail. |
|
|
Thanks again @TheRealMDoerr. All failures in your results are caused by missing optimizations (due to the limitations in the analysis) and not by correctness issues. Perhaps we could (in a future PR) split the test file into these two categories, and enable the correctness tests on all platforms. |
|
Hello, I am trying this on my linux-riscv64 platform. Just 10min please. |
|
The test results on linux-riscv64 show that only 'TestZGCBarrierElision.testAllocateArrayThenStoreAtUnknownIndex' failed. |
That's correct @RealFYang, thanks for the feedback! |
|
/integrate |
|
Going to push as commit dc9fe9a.
Your commit was automatically rebased without conflicts. |
|
@robcasloz Pushed as commit dc9fe9a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This changeset extends barrier elision analysis to handle atomic (x64, riscv, aarch64) and volatile (aarch64) memory access instructions. Offset-based addressing modes are unavailable or disabled for these instructions, which defeats the regular address component analysis (finding the base and offset of an address) that is central to barrier elision analysis. The changeset extends the address component analysis with special handling to detect and extract the component information from the inputs and Ideal type of the address computation node. The proposed extension relies on the fact that AddP-matched Mach nodes contain the base address in the same input slot, as guaranteed by Matcher::ReduceInst().
The changeset also adds additional test cases that exercise analysis of atomic accesses on arrays, with the goal of testing more complex address computations, and enables the tests in aarch64. @TheRealMDoerr, @RealFYang: please let me know if you want to try out the tests on the other ZGC-supporting architectures and enable them as part of this changeset or handle that later.
Testing: tier1-7 (x64 and aaarch64; linux, windows, and macosx; release and debug mode)
Alternative solutions
Three alternative solutions were explored and discarded in favor of this one:
Re-enable addressing modes for x64 atomics by duplicating the ADL instruction rules (see prototype). This enables the regular address component analysis for x64 but does not improve the situation for the other architectures. Furthermore, it makes the ZGC-specific ADL code less maintainable.
Re-enable addressing modes for x64 atomics by introducing an ADL construct (e.g. an "operand effect") to enforce that a certain operand is always assigned a distinct register from the other operands. This has the same effect as the above alternative without affecting the readability of the ZGC-specific ADL code, but it still does not solve the issue for the other architectures.
Extend the address component analysis with special handling as in this changeset, but performing a local analysis of the input address computation node (see prototype). This has a similar effect as this changeset for x64 but is less effective for other architectures with simpler addressing modes such as aarch64, where address computations are sometimes performed in multiple steps.
An open question is whether the local scope of the regular address component analysis affects the effectiveness of barrier elision for regular (non-atomic, non-volatile) memory accesses in architectures with more limited addressing modes than x64.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/zgc pull/13/head:pull/13$ git checkout pull/13Update a local copy of the PR:
$ git checkout pull/13$ git pull https://git.openjdk.org/zgc pull/13/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13View PR using the GUI difftool:
$ git pr show -t 13Using diff file
Download this PR as a diff file:
https://git.openjdk.org/zgc/pull/13.diff