-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8346184: C2: assert(has_node(i)) failed during split thru phi #22818
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
@rwestrel 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 90 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 |
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 reasonable. Does the new node come from here?
jdk/src/hotspot/share/opto/memnode.cpp
Lines 1211 to 1215 in f6e7713
if (ReduceBulkZeroing || find_array_copy_clone(ld_alloc, in(MemNode::Memory)) == nullptr) { | |
// If ReduceBulkZeroing is disabled, we need to check if the allocation does not belong to an | |
// ArrayCopyNode clone. If it does, then we cannot assume zero since the initialization is done | |
// by the ArrayCopyNode. | |
return phase->zerocon(memory_type()); |
Should we generally add some verification code that Identity()
does not create new nodes? Hard to do it at all places but it could, for example, be done in the transform()
methods of GVN and IGVN when calling Identity()
. But of course, should probably be done in a separate RFE.
The new test is failing with the following flags on linux-x64-debug:
Failure:
|
The assert fires during split thru phi because a call to
Identity
returns a new node (a constant null pointer). That happens because a
Load
, once pushed thru phi, can be constant folded because it loadsfrom a newly allocated array.
Identity
shouldn't return newnodes. When split thru phi runs, in this case,
Value
should be theone returning constant null, not
Identity
. There is logic for thatin
LoadNode::Value
but it's after some other checks that causeValue
to return too early.To fix this, I propose reordering checks in
LoadNode::Value
.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22818/head:pull/22818
$ git checkout pull/22818
Update a local copy of the PR:
$ git checkout pull/22818
$ git pull https://git.openjdk.org/jdk.git pull/22818/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22818
View PR using the GUI difftool:
$ git pr show -t 22818
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22818.diff
Using Webrev
Link to Webrev Comment