-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8351468: C2: array fill optimization assigns wrong type to intrinsic call #24005
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
8351468: C2: array fill optimization assigns wrong type to intrinsic call #24005
Conversation
|
👋 Welcome back rcastanedalo! A progress list of the required criteria for merging this PR into |
|
@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 135 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 |
|
@robcasloz The following label will be automatically applied to this pull request:
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. |
shipilev
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.
Wow, nice landmine.
(If you pull from current master, GHA should become clean)
Webrevs
|
|
I think the issue here is the implementation of |
I agree, in particular the fact that
A store of a |
No, a code such as this |
Done (commit 90fd766), thanks. |
Right, in this case I interpret from the comment at the declaration of |
|
@robcasloz I disagree, I would expect the |
Why not do this in this PR? Seems like the right approach to me. |
My thinking is that this is a bug whose fix we might want to backport to several JDK Update releases. The fix proposed in this PR is minimal and local to the array fill optimization, whereas the alternative approach of defining a
See the OpenJDK Developers' Guide for a more elaborate discussion of the trade-offs involved in backporting. Having said this, I still think we should consider introducing a |
If that was the intended meaning of |
|
@robcasloz Yes that's right. Then
To be clear, I don't think having |
Yes, that matches my understanding.
I agree, it would be good to do this (in a follow-up RFE). I like
I agree that using The alternative of using |
After some more thought, I lean towards just disabling the |
…and negative tests
Done now, and also added a set of positive and negative test cases (commit 38c9b47) and updated the PR description. @merykitty hopefully this addresses your concerns, please let me know what you think. |
eme64
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.
@robcasloz Nice catch, I'm glad you dug this up and found a reproducer 🥳
Yes, taking the element type from the address is the best, that way you actually depend on the array, not the type of the store.
| * @summary Test that loads anti-dependent on array fill intrinsics are | ||
| * scheduled correctly, for different load and array fill types. | ||
| * See detailed comments in testShort() below. | ||
| * @requires vm.compiler2.enabled |
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.
Is the test so expensive that C2 is required? Or can you just put -XX:+IgnoreUnrecognizedVMOptions in the run that has C2 flags?
| // Disabling unrolling is necessary for test robustness, otherwise the | ||
| // compiler might decide to unroll the array-filling loop instead of | ||
| // replacing it with an intrinsic call even if OptimizeFill is enabled. | ||
| TestFramework.runWithFlags("-XX:LoopUnrollLimit=0", "-XX:+OptimizeFill"); |
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.
What about a run without flags just in case?
| // Disabling unrolling is necessary for test robustness, otherwise the | ||
| // compiler might decide to unroll the array-filling loop instead of | ||
| // replacing it with an intrinsic call even if OptimizeFill is enabled. | ||
| TestFramework.runWithFlags("-XX:LoopUnrollLimit=0", "-XX:+OptimizeFill"); |
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.
What about a run without flags just in case?
TobiHartmann
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.
Good catch and nice tests! The fix looks good to me.
merykitty
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.
@robcasloz My concern is that MemNode::memory_type does not do what it seems to do. I wonder if there are other places misusing this method. The concern is orthogonal to this issue, though.
|
@merykitty You are right, Quickly looking at the cases, there are not even that many usages: Well, I looked through them, and I cannot see any issue with the other cases. But maybe someone else can give the usages a quick look too. |
@merykitty had the suggestions |
|
@TobiHartmann @merykitty @eme64 Thanks for reviewing! I will update the tests as suggested by @eme64 and re-run testing over the weekend. |
@merykitty @robcasloz |
Done: JDK-8352620. |
|
@RealFYang I enabled the new IR tests in Hi, Thanks for the ping. Yes, both of the newly-added tests are good on linux-riscv64 platform using fastdebug build. Great! |
eme64
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.
@robcasloz Thanks for the changes!
Some thoughts about future work on intrinsic fill.
- It would be nice to enable mismatched cases.
- And it would be nice to enable not just arrays, but also native memory. That would be especially good for MemorySegments. But not sure how easy this change would be.
| if (msg == nullptr && store->as_Mem()->is_mismatched_access()) { | ||
| msg = "mismatched store"; | ||
| } |
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.
What effect does this have?
Ah, it seems to have to do with these comments in your PR:
Additionally, the changeset makes it easier to reason about correctness by explicitly disabling the optimization for mismatched stores (where the type of the value to be stored differs from the element type of the destination array). Such stores were not optimized before, but only due to pattern matching limitations.
It may be good to leave additional comments in the code here, saying that this is a limitation, and maybe improved in the future. Up to you.
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.
I agree, done in commit c0b3cf9.
|
Thanks for re-reviewing and the additional suggestions @eme64! |
|
/integrate |
|
Going to push as commit de58009.
Your commit was automatically rebased without conflicts. |
|
@robcasloz Pushed as commit de58009. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The array fill optimization replaces simple innermost loops that fill an array with copies of the same primitive value:
with a call to an array filling intrinsic that is specialized for the array element type:
The optimization retrieves the (basic) array element type from calling
MemNode::memory_type()on the original filling store. This is incorrect for stores ofshortvalues, since these are represented byStoreCnodes whosememory_type()isT_CHAR. As a result, the optimization wrongly assigns the address typechar[]toshortarray fill loops. This can cause miscompilations due to missing anti-dependences, see the issue description for further detail.This changeset proposes retrieving the (basic) array element type from the store address type instead. This ensures that the accurate address type is assigned to the intrinsic call, preventing missed anti-dependences and other potential issues caused by mismatching types. Additionally, the changeset makes it easier to reason about correctness by explicitly disabling the optimization for mismatched stores (where the type of the value to be stored differs from the element type of the destination array). Such stores were not optimized before, but only due to pattern matching limitations.
Assuming mismatched stores are discarded (as proposed here), an alternative solution would be to define a StoreS node returning the appropriate
memory_type(). This could be desirable even as a complement to this fix, to prevent similar bugs in the future. I propose to investigate the introduction of a StoreS node in a separate RFE, because it is a much larger and more intrusive changeset, and go with this minimal, local, and non-intrusive fix for backportability.Testing: tier1-5, stress testing (linux-x64, windows-x64, macosx-x64, linux-aarch64, macosx-aarch64).
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24005/head:pull/24005$ git checkout pull/24005Update a local copy of the PR:
$ git checkout pull/24005$ git pull https://git.openjdk.org/jdk.git pull/24005/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24005View PR using the GUI difftool:
$ git pr show -t 24005Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24005.diff
Using Webrev
Link to Webrev Comment