stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8248791: sun/util/resources/cldr/TimeZoneNamesTest.java fails with -XX:-ReduceInitialCardMarks -XX:-ReduceBulkZeroing

Browse Source 
Fix wrong replacement of loads by zero for non-completed InitializationNodes belonging to a clone when ReduceBulkZeroing is disabled.

Reviewed-by: kvn, thartmann
This commit is contained in:
Christian Hagedorn 2020-08-14 10:30:51 +02:00
parent 90f0612ada
commit 552a73301c
3 changed files with 142 additions and 27 deletions
src/hotspot/share/opto
memnode.cppmemnode.hpp
test/hotspot/jtreg/compiler/arraycopy
TestCloneAccess.java

67
src/hotspot/share/opto/memnode.cpp

@ -530,32 +530,9 @@ bool MemNode::detect_ptr_independence(Node* p1, AllocateNode* a1,
// Find an arraycopy that must have set (can_see_stored_value=true) or
// could have set (can_see_stored_value=false) the value for this load
Node* LoadNode::find_previous_arraycopy(PhaseTransform* phase, Node* ld_alloc, Node*& mem, bool can_see_stored_value) const {
if (mem->is_Proj() && mem->in(0) != NULL && (mem->in(0)->Opcode() == Op_MemBarStoreStore ||
mem->in(0)->Opcode() == Op_MemBarCPUOrder)) {
if (ld_alloc != NULL) {
// Check if there is an array copy for a clone
Node* mb = mem->in(0);
ArrayCopyNode* ac = NULL;
if (mb->in(0) != NULL && mb->in(0)->is_Proj() &&
mb->in(0)->in(0) != NULL && mb->in(0)->in(0)->is_ArrayCopy()) {
ac = mb->in(0)->in(0)->as_ArrayCopy();
} else {
// Step over GC barrier when ReduceInitialCardMarks is disabled
BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
Node* control_proj_ac = bs->step_over_gc_barrier(mb->in(0));
if (control_proj_ac->is_Proj() && control_proj_ac->in(0)->is_ArrayCopy()) {
ac = control_proj_ac->in(0)->as_ArrayCopy();
}
}
if (ac != NULL && ac->is_clonebasic()) {
AllocateNode* alloc = AllocateNode::Ideal_allocation(ac->in(ArrayCopyNode::Dest), phase);
if (alloc != NULL && alloc == ld_alloc) {
return ac;
}
}
}
ArrayCopyNode* ac = find_array_copy_clone(phase, ld_alloc, mem);
if (ac != NULL) {
return ac;
} else if (mem->is_Proj() && mem->in(0) != NULL && mem->in(0)->is_ArrayCopy()) {
ArrayCopyNode* ac = mem->in(0)->as_ArrayCopy();
@ -584,6 +561,37 @@ Node* LoadNode::find_previous_arraycopy(PhaseTransform* phase, Node* ld_alloc, N
return NULL;
}
ArrayCopyNode* MemNode::find_array_copy_clone(PhaseTransform* phase, Node* ld_alloc, Node* mem) const {
if (mem->is_Proj() && mem->in(0) != NULL && (mem->in(0)->Opcode() == Op_MemBarStoreStore ||
mem->in(0)->Opcode() == Op_MemBarCPUOrder)) {
if (ld_alloc != NULL) {
// Check if there is an array copy for a clone
Node* mb = mem->in(0);
ArrayCopyNode* ac = NULL;
if (mb->in(0) != NULL && mb->in(0)->is_Proj() &&
mb->in(0)->in(0) != NULL && mb->in(0)->in(0)->is_ArrayCopy()) {
ac = mb->in(0)->in(0)->as_ArrayCopy();
} else {
// Step over GC barrier when ReduceInitialCardMarks is disabled
BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
Node* control_proj_ac = bs->step_over_gc_barrier(mb->in(0));
if (control_proj_ac->is_Proj() && control_proj_ac->in(0)->is_ArrayCopy()) {
ac = control_proj_ac->in(0)->as_ArrayCopy();
}
}
if (ac != NULL && ac->is_clonebasic()) {
AllocateNode* alloc = AllocateNode::Ideal_allocation(ac->in(ArrayCopyNode::Dest), phase);
if (alloc != NULL && alloc == ld_alloc) {
return ac;
}
}
}
}
return NULL;
}
// The logic for reordering loads and stores uses four steps:
// (a) Walk carefully past stores and initializations which we
// can prove are independent of this load.
@ -1103,7 +1111,12 @@ Node* MemNode::can_see_stored_value(Node* st, PhaseTransform* phase) const {
// (This is one of the few places where a generic PhaseTransform
// can create new nodes. Think of it as lazily manifesting
// virtually pre-existing constants.)
return phase->zerocon(memory_type());
if (ReduceBulkZeroing || find_array_copy_clone(phase, ld_alloc, in(MemNode::Memory)) == NULL) {
// 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());
}
}
// A load from an initialization barrier can match a captured store.

1
src/hotspot/share/opto/memnode.hpp

@ -93,6 +93,7 @@ protected:
}
virtual Node* find_previous_arraycopy(PhaseTransform* phase, Node* ld_alloc, Node*& mem, bool can_see_stored_value) const { return NULL; }
ArrayCopyNode* find_array_copy_clone(PhaseTransform* phase, Node* ld_alloc, Node* mem) const;
static bool check_if_adr_maybe_raw(Node* adr);
public:

101
test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java Normal file

@ -0,0 +1,101 @@
/*
* Copyright (c) 2020, 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 8248791
* @summary Test cloning with more than 8 (=ArrayCopyLoadStoreMaxElem) where loads are wrongly replaced by zero.
* @run main/othervm -XX:-ReduceBulkZeroing
* -XX:CompileCommand=dontinline,compiler.arraycopy.TestCloneAccess::*
* compiler.arraycopy.TestCloneAccess
* @run main/othervm -XX:-ReduceBulkZeroing -XX:-ReduceInitialCardMarks
* -XX:CompileCommand=dontinline,compiler.arraycopy.TestCloneAccess::*
* compiler.arraycopy.TestCloneAccess
*/
package compiler.arraycopy;
public class TestCloneAccess {
static int test(E src) throws CloneNotSupportedException {
// ArrayCopyNode for this clone is not inlined since there are more than 8 (=ArrayCopyLoadStoreMaxElem) fields
src.i1 = 3;
E dest = (E)src.clone();
dontInline(dest.i1, dest.i2);
// Both loads are wrongly optimized and replaced by a constant zero. LoadNode::Value() tries to find out if a load
// is done from a freshly-allocated object. If that is the case, the load can be replaced by the default value zero.
// However, in this case, the Allocation node belongs to an ArrayCopyNode which is responsible for initializing 'dest'.
// If -XX:-ReduceBulkZeroing is set, the InitializationNode of the allocation does not bail out of this optimization
// which results in a replacement of both loads by zero. This is addressed by this fix. If -XX:+ReduceBulkZeroing is
// set, then we already bail out and perform the load correctly.
return dest.i1 + dest.i2;
}
public static void main(String[] args) throws Exception {
E e = new E();
e.i2 = 4;
int res = 0;
for (int i = 0; i < 20000; i++) {
res = test(e);
if (res != 7 || e.i1 != 3 || e.i2 != 4) {
throw new RuntimeException("Wrong result! Expected: res = 7, e.i1 = 3, e.i2 = 4 "
+ "but got: res = " + res + ", e.i1 = " + e.i1 + ", e.i2 = " + e.i2);
}
}
}
// Dont inline this method
public static void dontInline(int i1, int i2) {
}
}
class E implements Cloneable {
/*
* Need more than 8 (=ArrayCopyLoadStoreMaxElem) fields
*/
int i1;
int i2;
int i3;
int i4;
int i5;
int i6;
int i7;
int i8;
int i9;
E() {
i1 = 0;
i2 = 1;
i3 = 2;
i4 = 3;
i5 = 4;
i6 = 5;
i7 = 6;
i8 = 7;
i9 = 8;
}
public Object clone() throws CloneNotSupportedException {
return super.clone();
}
}