# HG changeset patch # User roland # Date 1361797984 -3600 # Node ID 6931f425c517f6c2df735605f2d9c42c74618147 # Parent be1fbee2076592e72e391a9a11fdeba42161c1e3 8007294: ReduceFieldZeroing doesn't check for dependent load and can lead to incorrect execution Summary: InitializeNode::can_capture_store() must check that the captured store doesn't overwrite a memory location that is loaded before the store. Reviewed-by: kvn diff -r be1fbee20765 -r 6931f425c517 src/share/vm/opto/memnode.cpp --- a/src/share/vm/opto/memnode.cpp Fri Feb 22 10:12:00 2013 -0800 +++ b/src/share/vm/opto/memnode.cpp Mon Feb 25 14:13:04 2013 +0100 @@ -320,6 +320,9 @@ if (mem != old_mem) { set_req(MemNode::Memory, mem); + if (can_reshape && old_mem->outcnt() == 0) { + igvn->_worklist.push(old_mem); + } if (phase->type( mem ) == Type::TOP) return NodeSentinel; return this; } @@ -2319,9 +2322,9 @@ if (ReduceFieldZeroing && /*can_reshape &&*/ mem->is_Proj() && mem->in(0)->is_Initialize()) { InitializeNode* init = mem->in(0)->as_Initialize(); - intptr_t offset = init->can_capture_store(this, phase); + intptr_t offset = init->can_capture_store(this, phase, can_reshape); if (offset > 0) { - Node* moved = init->capture_store(this, offset, phase); + Node* moved = init->capture_store(this, offset, phase, can_reshape); // If the InitializeNode captured me, it made a raw copy of me, // and I need to disappear. if (moved != NULL) { @@ -3134,7 +3137,7 @@ // an initialization. Returns zero if a check fails. // On success, returns the (constant) offset to which the store applies, // within the initialized memory. -intptr_t InitializeNode::can_capture_store(StoreNode* st, PhaseTransform* phase) { +intptr_t InitializeNode::can_capture_store(StoreNode* st, PhaseTransform* phase, bool can_reshape) { const int FAIL = 0; if (st->req() != MemNode::ValueIn + 1) return FAIL; // an inscrutable StoreNode (card mark?) @@ -3156,6 +3159,91 @@ if (!detect_init_independence(val, true, complexity_count)) return FAIL; // stored value must be 'simple enough' + // The Store can be captured only if nothing after the allocation + // and before the Store is using the memory location that the store + // overwrites. + bool failed = false; + // If is_complete_with_arraycopy() is true the shape of the graph is + // well defined and is safe so no need for extra checks. + if (!is_complete_with_arraycopy()) { + // We are going to look at each use of the memory state following + // the allocation to make sure nothing reads the memory that the + // Store writes. + const TypePtr* t_adr = phase->type(adr)->isa_ptr(); + int alias_idx = phase->C->get_alias_index(t_adr); + ResourceMark rm; + Unique_Node_List mems; + mems.push(mem); + Node* unique_merge = NULL; + for (uint next = 0; next < mems.size(); ++next) { + Node *m = mems.at(next); + for (DUIterator_Fast jmax, j = m->fast_outs(jmax); j < jmax; j++) { + Node *n = m->fast_out(j); + if (n->outcnt() == 0) { + continue; + } + if (n == st) { + continue; + } else if (n->in(0) != NULL && n->in(0) != ctl) { + // If the control of this use is different from the control + // of the Store which is right after the InitializeNode then + // this node cannot be between the InitializeNode and the + // Store. + continue; + } else if (n->is_MergeMem()) { + if (n->as_MergeMem()->memory_at(alias_idx) == m) { + // We can hit a MergeMemNode (that will likely go away + // later) that is a direct use of the memory state + // following the InitializeNode on the same slice as the + // store node that we'd like to capture. We need to check + // the uses of the MergeMemNode. + mems.push(n); + } + } else if (n->is_Mem()) { + Node* other_adr = n->in(MemNode::Address); + if (other_adr == adr) { + failed = true; + break; + } else { + const TypePtr* other_t_adr = phase->type(other_adr)->isa_ptr(); + if (other_t_adr != NULL) { + int other_alias_idx = phase->C->get_alias_index(other_t_adr); + if (other_alias_idx == alias_idx) { + // A load from the same memory slice as the store right + // after the InitializeNode. We check the control of the + // object/array that is loaded from. If it's the same as + // the store control then we cannot capture the store. + assert(!n->is_Store(), "2 stores to same slice on same control?"); + Node* base = other_adr; + assert(base->is_AddP(), err_msg_res("should be addp but is %s", base->Name())); + base = base->in(AddPNode::Base); + if (base != NULL) { + base = base->uncast(); + if (base->is_Proj() && base->in(0) == alloc) { + failed = true; + break; + } + } + } + } + } + } else { + failed = true; + break; + } + } + } + } + if (failed) { + if (!can_reshape) { + // We decided we couldn't capture the store during parsing. We + // should try again during the next IGVN once the graph is + // cleaner. + phase->C->record_for_igvn(st); + } + return FAIL; + } + return offset; // success } @@ -3266,11 +3354,11 @@ // rawstore1 rawstore2) // Node* InitializeNode::capture_store(StoreNode* st, intptr_t start, - PhaseTransform* phase) { + PhaseTransform* phase, bool can_reshape) { assert(stores_are_sane(phase), ""); if (start < 0) return NULL; - assert(can_capture_store(st, phase) == start, "sanity"); + assert(can_capture_store(st, phase, can_reshape) == start, "sanity"); Compile* C = phase->C; int size_in_bytes = st->memory_size(); diff -r be1fbee20765 -r 6931f425c517 src/share/vm/opto/memnode.hpp --- a/src/share/vm/opto/memnode.hpp Fri Feb 22 10:12:00 2013 -0800 +++ b/src/share/vm/opto/memnode.hpp Mon Feb 25 14:13:04 2013 +0100 @@ -1072,11 +1072,11 @@ // See if this store can be captured; return offset where it initializes. // Return 0 if the store cannot be moved (any sort of problem). - intptr_t can_capture_store(StoreNode* st, PhaseTransform* phase); + intptr_t can_capture_store(StoreNode* st, PhaseTransform* phase, bool can_reshape); // Capture another store; reformat it to write my internal raw memory. // Return the captured copy, else NULL if there is some sort of problem. - Node* capture_store(StoreNode* st, intptr_t start, PhaseTransform* phase); + Node* capture_store(StoreNode* st, intptr_t start, PhaseTransform* phase, bool can_reshape); // Find captured store which corresponds to the range [start..start+size). // Return my own memory projection (meaning the initial zero bits) diff -r be1fbee20765 -r 6931f425c517 src/share/vm/opto/node.cpp --- a/src/share/vm/opto/node.cpp Fri Feb 22 10:12:00 2013 -0800 +++ b/src/share/vm/opto/node.cpp Mon Feb 25 14:13:04 2013 +0100 @@ -1261,6 +1261,7 @@ if (dead->is_expensive()) { igvn->C->remove_expensive_node(dead); } + igvn->C->record_dead_node(dead->_idx); // Kill all inputs to the dead guy for (uint i=0; i < dead->req(); i++) { Node *n = dead->in(i); // Get input to dead guy diff -r be1fbee20765 -r 6931f425c517 src/share/vm/opto/phaseX.cpp --- a/src/share/vm/opto/phaseX.cpp Fri Feb 22 10:12:00 2013 -0800 +++ b/src/share/vm/opto/phaseX.cpp Mon Feb 25 14:13:04 2013 +0100 @@ -1197,6 +1197,18 @@ assert(!(i < imax), "sanity"); } } + if (ReduceFieldZeroing && dead->is_Load() && i == MemNode::Memory && + in->is_Proj() && in->in(0) != NULL && in->in(0)->is_Initialize()) { + // A Load that directly follows an InitializeNode is + // going away. The Stores that follow are candidates + // again to be captured by the InitializeNode. + for (DUIterator_Fast jmax, j = in->fast_outs(jmax); j < jmax; j++) { + Node *n = in->fast_out(j); + if (n->is_Store()) { + _worklist.push(n); + } + } + } } } C->record_dead_node(dead->_idx); diff -r be1fbee20765 -r 6931f425c517 test/compiler/8007294/Test8007294.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/compiler/8007294/Test8007294.java Mon Feb 25 14:13:04 2013 +0100 @@ -0,0 +1,98 @@ +/* + * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8007294 + * @summary ReduceFieldZeroing doesn't check for dependent load and can lead to incorrect execution + * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+AlwaysIncrementalInline -XX:-UseOnStackReplacement -XX:-BackgroundCompilation Test8007294 + * + */ + +public class Test8007294 { + + int i1; + int i2; + + Test8007294(int i1, int i2) { + this.i1 = i1; + this.i2 = i2; + } + + static int m(int v) { + return v; + } + + static Test8007294 test1() { + Test8007294 obj = new Test8007294(10, 100); + int v1 = obj.i1; + + int v3 = m(v1); + int v2 = obj.i2; + obj.i2 = v3; + obj.i1 = v2; + + return obj; + } + + static int test2(int i) { + int j = 0; + if (i > 0) { + j = 1; + } + + int[] arr = new int[10]; + arr[0] = 1; + arr[1] = 2; + int v1 = arr[j]; + arr[0] = 3; + arr[1] = 4; + + return v1; + } + + static public void main(String[] args) { + boolean failed = false; + for (int i = 0; i < 20000; i++) { + Test8007294 obj = test1(); + if (obj.i1 != 100 || obj.i2 != 10) { + System.out.println("FAILED test1 obj.i1 = " + obj.i1 +", obj.i2 = " + obj.i2); + failed = true; + break; + } + } + for (int i = 0; i < 20000; i++) { + int res = test2(1); + if (res != 2) { + System.out.println("FAILED test2 = " + res); + failed = true; + break; + } + } + if (failed) { + System.exit(97); + } else { + System.out.println("PASSED"); + } + } +}