-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed #24570
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
9f220f4
5f8e5bf
5041f64
dfa5da2
125f970
71fd831
a4031f3
c32e453
082dcb5
7139deb
469d8a5
1639264
377a8d7
a32b1d2
7afc47e
17517ae
f22471c
a401bed
624d69e
6134c97
6a01123
a76430b
9f3b9cf
02b43b7
50e9437
23035e0
80d562a
d5a2738
af8480c
080638c
616678f
2a451c6
a6c6c04
43c6f82
c0a8ad2
4b656f2
24ff0e2
c189a7a
69c6e50
3b5b54a
ec9f278
f528ce8
fc13578
b066f3c
b701d03
c64d68c
0ff5c45
46972dc
917ea91
2207487
f871390
6ea8c81
9fd8dc1
f12efbc
48257c9
b5ac753
957be06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,7 +167,7 @@ class MemNode : public Node { | |
| bool is_unsafe_access() const { return _unsafe_access; } | ||
|
|
||
| #ifndef PRODUCT | ||
| static void dump_adr_type(const Node* mem, const TypePtr* adr_type, outputStream *st); | ||
| static void dump_adr_type(const TypePtr* adr_type, outputStream* st); | ||
| virtual void dump_spec(outputStream *st) const; | ||
| #endif | ||
| }; | ||
|
|
@@ -1380,6 +1380,8 @@ class InitializeNode: public MemBarNode { | |
| // Does a NarrowMemProj with this adr_type and this node as input already exist? | ||
| bool already_has_narrow_mem_proj_with_adr_type(const TypePtr* adr_type) const; | ||
|
|
||
| // Used during matching: find the MachProj memory projection if there's one. Expectation is that there should be at | ||
| // most one. | ||
| MachProjNode* mem_mach_proj() const; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a brief comment above this function, possibly clarifying that we do not expect to find more than one Mach memory projection. |
||
|
|
||
| private: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2592,15 +2592,15 @@ void PhaseIterGVN::add_users_of_use_to_worklist(Node* n, Node* use, Unique_Node_ | |
| } | ||
| } | ||
| } | ||
| auto enqueue_init_mem_projs = [&](ProjNode* proj) { | ||
| add_users_to_worklist0(proj, worklist); | ||
| return MultiNode::CONTINUE; | ||
| }; | ||
|
Comment on lines
2595
to
2597
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| // If changed initialization activity, check dependent Stores | ||
| if (use_op == Op_Allocate || use_op == Op_AllocateArray) { | ||
| InitializeNode* init = use->as_Allocate()->initialization(); | ||
| if (init != nullptr) { | ||
| auto enqueue = [&](ProjNode* proj) { | ||
| add_users_to_worklist0(proj, worklist); | ||
| return MultiNode::CONTINUE; | ||
| }; | ||
| init->apply_to_projs(enqueue, TypeFunc::Memory); | ||
| init->apply_to_projs(enqueue_init_mem_projs, TypeFunc::Memory); | ||
| } | ||
| } | ||
| // If the ValidLengthTest input changes then the fallthrough path out of the AllocateArray may have become dead. | ||
|
|
@@ -2615,11 +2615,7 @@ void PhaseIterGVN::add_users_of_use_to_worklist(Node* n, Node* use, Unique_Node_ | |
|
|
||
| if (use_op == Op_Initialize) { | ||
| InitializeNode* init = use->as_Initialize(); | ||
| auto enqueue = [&](ProjNode* proj) { | ||
| add_users_to_worklist0(proj, worklist); | ||
| return MultiNode::CONTINUE; | ||
| }; | ||
| init->apply_to_projs(enqueue, TypeFunc::Memory); | ||
| init->apply_to_projs(enqueue_init_mem_projs, TypeFunc::Memory); | ||
| } | ||
| // Loading the java mirror from a Klass requires two loads and the type | ||
| // of the mirror load depends on the type of 'n'. See LoadNode::Value(). | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a package declaration (and make the corresponding class names fully qualified in 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.
Does the same argument as below apply for relaxing the scope of this memory barrier? Please clarify in a similar comment for this case (if the same argument applies, a reference to the comment below would be enough).
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.
Thanks for adding the comment. A follow-up question: the full comment below makes the argument that re-ordering by the compiler can't happen by construction because a later Store that publishes the just allocated object reference is indirectly control dependent on the Initialize node. However, in this case, there may be no such Initialize node (
init == nullptr || init->req() < InitializeNode::RawStores). I assume the memory barrier relaxation is still OK in this scenario because we cannot have later, publishing stores of the allocated object reference? That is, if there exists such a store then there must necessarily exist an Initialize node? Or is there any other reason I am missing? It would be good to clarify this point in the 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.
I updated the comment. Can you have a look?
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.
Thanks for the clarification and the pointer to the comment in
initialize_object, that helps.