From 8364a7d75257fc84891f5bf512a345e66ad36a49 Mon Sep 17 00:00:00 2001 From: poonam Date: Fri, 28 Oct 2016 22:36:23 +0000 Subject: [PATCH] 8038348: Instance field load is replaced by wrong data Phi Summary: Store additional information in PhiNodes corresponding to known instance field values to avoid incorrect reusage. Reviewed-by: kvn, thartmann --- src/share/vm/opto/cfgnode.hpp | 10 +++++++++- src/share/vm/opto/macro.cpp | 4 ++-- src/share/vm/opto/memnode.cpp | 4 ++-- src/share/vm/opto/phaseX.cpp | 14 ++++++++++++++ src/share/vm/opto/type.hpp | 2 +- 5 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/share/vm/opto/cfgnode.hpp b/src/share/vm/opto/cfgnode.hpp index e795483e3..74461aa85 100644 --- a/src/share/vm/opto/cfgnode.hpp +++ b/src/share/vm/opto/cfgnode.hpp @@ -119,6 +119,9 @@ class JProjNode : public ProjNode { // input in slot 0. class PhiNode : public TypeNode { const TypePtr* const _adr_type; // non-null only for Type::MEMORY nodes. + // The following fields are only used for data PhiNodes to indicate + // that the PhiNode represents the value of a known instance field. + int _inst_mem_id; // Instance memory id (node index of the memory Phi) const int _inst_id; // Instance id of the memory slice. const int _inst_index; // Alias index of the instance memory slice. // Array elements references have the same alias_idx but different offset. @@ -138,11 +141,13 @@ public: }; PhiNode( Node *r, const Type *t, const TypePtr* at = NULL, + const int imid = -1, const int iid = TypeOopPtr::InstanceTop, const int iidx = Compile::AliasIdxTop, const int ioffs = Type::OffsetTop ) : TypeNode(t,r->req()), _adr_type(at), + _inst_mem_id(imid), _inst_id(iid), _inst_index(iidx), _inst_offset(ioffs) @@ -187,11 +192,14 @@ public: virtual bool pinned() const { return in(0) != 0; } virtual const TypePtr *adr_type() const { verify_adr_type(true); return _adr_type; } + void set_inst_mem_id(int inst_mem_id) { _inst_mem_id = inst_mem_id; } + const int inst_mem_id() const { return _inst_mem_id; } const int inst_id() const { return _inst_id; } const int inst_index() const { return _inst_index; } const int inst_offset() const { return _inst_offset; } - bool is_same_inst_field(const Type* tp, int id, int index, int offset) { + bool is_same_inst_field(const Type* tp, int mem_id, int id, int index, int offset) { return type()->basic_type() == tp->basic_type() && + inst_mem_id() == mem_id && inst_id() == id && inst_index() == index && inst_offset() == offset && diff --git a/src/share/vm/opto/macro.cpp b/src/share/vm/opto/macro.cpp index 01b329c73..d1e1f3220 100644 --- a/src/share/vm/opto/macro.cpp +++ b/src/share/vm/opto/macro.cpp @@ -401,7 +401,7 @@ Node *PhaseMacroExpand::value_from_mem_phi(Node *mem, BasicType ft, const Type * for (DUIterator_Fast kmax, k = region->fast_outs(kmax); k < kmax; k++) { Node* phi = region->fast_out(k); if (phi->is_Phi() && phi != mem && - phi->as_Phi()->is_same_inst_field(phi_type, instance_id, alias_idx, offset)) { + phi->as_Phi()->is_same_inst_field(phi_type, (int)mem->_idx, instance_id, alias_idx, offset)) { return phi; } } @@ -420,7 +420,7 @@ Node *PhaseMacroExpand::value_from_mem_phi(Node *mem, BasicType ft, const Type * GrowableArray values(length, length, NULL, false); // create a new Phi for the value - PhiNode *phi = new (C) PhiNode(mem->in(0), phi_type, NULL, instance_id, alias_idx, offset); + PhiNode *phi = new (C) PhiNode(mem->in(0), phi_type, NULL, mem->_idx, instance_id, alias_idx, offset); transform_later(phi); value_phis->push(phi, mem->_idx); diff --git a/src/share/vm/opto/memnode.cpp b/src/share/vm/opto/memnode.cpp index 80e25c944..a922a4efb 100644 --- a/src/share/vm/opto/memnode.cpp +++ b/src/share/vm/opto/memnode.cpp @@ -1155,7 +1155,7 @@ Node *LoadNode::Identity( PhaseTransform *phase ) { for (DUIterator_Fast imax, i = region->fast_outs(imax); i < imax; i++) { Node* phi = region->fast_out(i); if (phi->is_Phi() && phi != mem && - phi->as_Phi()->is_same_inst_field(this_type, this_iid, this_index, this_offset)) { + phi->as_Phi()->is_same_inst_field(this_type, (int)mem->_idx, this_iid, this_index, this_offset)) { return phi; } } @@ -1400,7 +1400,7 @@ Node *LoadNode::split_through_phi(PhaseGVN *phase) { this_iid = base->_idx; } PhaseIterGVN* igvn = phase->is_IterGVN(); - Node* phi = new (C) PhiNode(region, this_type, NULL, this_iid, this_index, this_offset); + Node* phi = new (C) PhiNode(region, this_type, NULL, mem->_idx, this_iid, this_index, this_offset); for (uint i = 1; i < region->req(); i++) { Node* x; Node* the_clone = NULL; diff --git a/src/share/vm/opto/phaseX.cpp b/src/share/vm/opto/phaseX.cpp index f090fcca0..1782cb9cb 100644 --- a/src/share/vm/opto/phaseX.cpp +++ b/src/share/vm/opto/phaseX.cpp @@ -481,6 +481,8 @@ PhaseRenumberLive::PhaseRenumberLive(PhaseGVN* gvn, uint current_idx = 0; // The current new node ID. Incremented after every assignment. for (uint i = 0; i < _useful.size(); i++) { Node* n = _useful.at(i); + // Sanity check that fails if we ever decide to execute this phase after EA + assert(!n->is_Phi() || n->as_Phi()->inst_mem_id() == -1, "should not be linked to data Phi"); const Type* type = gvn->type_or_null(n); new_type_array.map(current_idx, type); @@ -1378,6 +1380,18 @@ void PhaseIterGVN::subsume_node( Node *old, Node *nn ) { i -= num_edges; // we deleted 1 or more copies of this edge } + // Search for instance field data PhiNodes in the same region pointing to the old + // memory PhiNode and update their instance memory ids to point to the new node. + if (old->is_Phi() && old->as_Phi()->type()->has_memory() && old->in(0) != NULL) { + Node* region = old->in(0); + for (DUIterator_Fast imax, i = region->fast_outs(imax); i < imax; i++) { + PhiNode* phi = region->fast_out(i)->isa_Phi(); + if (phi != NULL && phi->inst_mem_id() == (int)old->_idx) { + phi->set_inst_mem_id((int)nn->_idx); + } + } + } + // Smash all inputs to 'old', isolating him completely Node *temp = new (C) Node(1); temp->init_req(0,nn); // Add a use to nn to prevent him from dying diff --git a/src/share/vm/opto/type.hpp b/src/share/vm/opto/type.hpp index 4506c3a96..3c69445aa 100644 --- a/src/share/vm/opto/type.hpp +++ b/src/share/vm/opto/type.hpp @@ -882,7 +882,7 @@ protected: // If not InstanceTop or InstanceBot, indicates that this is // a particular instance of this type which is distinct. - // This is the the node index of the allocation node creating this instance. + // This is the node index of the allocation node creating this instance. int _instance_id; // Extra type information profiling gave us. We propagate it the -- GitLab