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, vlivanov
--- a/hotspot/src/share/vm/opto/cfgnode.hpp Mon Aug 22 20:30:37 2016 +0000
+++ b/hotspot/src/share/vm/opto/cfgnode.hpp Tue Aug 23 13:44:26 2016 +0200
@@ -119,6 +119,9 @@
// 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 @@
};
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)
@@ -194,11 +199,14 @@
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 &&
--- a/hotspot/src/share/vm/opto/macro.cpp Mon Aug 22 20:30:37 2016 +0000
+++ b/hotspot/src/share/vm/opto/macro.cpp Tue Aug 23 13:44:26 2016 +0200
@@ -491,7 +491,7 @@
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;
}
}
@@ -510,7 +510,7 @@
GrowableArray <Node *> values(length, length, NULL, false);
// create a new Phi for the value
- PhiNode *phi = new PhiNode(mem->in(0), phi_type, NULL, instance_id, alias_idx, offset);
+ PhiNode *phi = new PhiNode(mem->in(0), phi_type, NULL, mem->_idx, instance_id, alias_idx, offset);
transform_later(phi);
value_phis->push(phi, mem->_idx);
--- a/hotspot/src/share/vm/opto/memnode.cpp Mon Aug 22 20:30:37 2016 +0000
+++ b/hotspot/src/share/vm/opto/memnode.cpp Tue Aug 23 13:44:26 2016 +0200
@@ -1112,7 +1112,7 @@
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;
}
}
@@ -1395,7 +1395,7 @@
this_iid = base->_idx;
}
PhaseIterGVN* igvn = phase->is_IterGVN();
- Node* phi = new PhiNode(region, this_type, NULL, this_iid, this_index, this_offset);
+ Node* phi = new 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;
--- a/hotspot/src/share/vm/opto/phaseX.cpp Mon Aug 22 20:30:37 2016 +0000
+++ b/hotspot/src/share/vm/opto/phaseX.cpp Tue Aug 23 13:44:26 2016 +0200
@@ -491,6 +491,8 @@
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);
@@ -1448,6 +1450,18 @@
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 Node(1);
temp->init_req(0,nn); // Add a use to nn to prevent him from dying
--- a/hotspot/src/share/vm/opto/type.hpp Mon Aug 22 20:30:37 2016 +0000
+++ b/hotspot/src/share/vm/opto/type.hpp Tue Aug 23 13:44:26 2016 +0200
@@ -963,7 +963,7 @@
// 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;
static const TypeOopPtr* make_from_klass_common(ciKlass* klass, bool klass_change, bool try_for_exact);