mirror of
https://github.com/openjdk/jdk.git
synced 2026-05-20 10:27:52 +00:00
8370405: C2: mismatched store from MergeStores wrongly scalarized in allocation elimination
Co-authored-by: Olivier Mattmann <olivier.mattmann@bluewin.ch> Co-authored-by: Quan Anh Mai <qamai@openjdk.org> Reviewed-by: kvn, qamai
This commit is contained in:
Emanuel Peter
parent
0ca0852d78
commit
09a047f00c
@ -595,6 +595,11 @@ bool PhaseMacroExpand::can_eliminate_allocation(PhaseIterGVN* igvn, AllocateNode
|
||||
for (DUIterator_Fast kmax, k = use->fast_outs(kmax);
|
||||
k < kmax && can_eliminate; k++) {
|
||||
Node* n = use->fast_out(k);
|
||||
if (n->is_Mem() && n->as_Mem()->is_mismatched_access()) {
|
||||
DEBUG_ONLY(disq_node = n);
|
||||
NOT_PRODUCT(fail_eliminate = "Mismatched access");
|
||||
can_eliminate = false;
|
||||
}
|
||||
if (!n->is_Store() && n->Opcode() != Op_CastP2X && !bs->is_gc_pre_barrier_node(n) && !reduce_merge_precheck) {
|
||||
DEBUG_ONLY(disq_node = n;)
|
||||
if (n->is_Load() || n->is_LoadStore()) {
|
||||
@ -732,6 +737,41 @@ void PhaseMacroExpand::undo_previous_scalarizations(GrowableArray <SafePointNode
|
||||
}
|
||||
}
|
||||
|
||||
#ifdef ASSERT
|
||||
// Verify if a value can be written into a field.
|
||||
void verify_type_compatability(const Type* value_type, const Type* field_type) {
|
||||
BasicType value_bt = value_type->basic_type();
|
||||
BasicType field_bt = field_type->basic_type();
|
||||
|
||||
// Primitive types must match.
|
||||
if (is_java_primitive(value_bt) && value_bt == field_bt) { return; }
|
||||
|
||||
// I have been struggling to make a similar assert for non-primitive
|
||||
// types. I we can add one in the future. For now, I just let them
|
||||
// pass without checks.
|
||||
// In particular, I was struggling with a value that came from a call,
|
||||
// and had only a non-null check CastPP. There was also a checkcast
|
||||
// in the graph to verify the interface, but the corresponding
|
||||
// CheckCastPP result was not updated in the stack slot, and so
|
||||
// we ended up using the CastPP. That means that the field knows
|
||||
// that it should get an oop from an interface, but the value lost
|
||||
// that information, and so it is not a subtype.
|
||||
// There may be other issues, feel free to investigate further!
|
||||
if (!is_java_primitive(value_bt)) { return; }
|
||||
|
||||
tty->print_cr("value not compatible for field: %s vs %s",
|
||||
type2name(value_bt),
|
||||
type2name(field_bt));
|
||||
tty->print("value_type: ");
|
||||
value_type->dump();
|
||||
tty->cr();
|
||||
tty->print("field_type: ");
|
||||
field_type->dump();
|
||||
tty->cr();
|
||||
assert(false, "value_type does not fit field_type");
|
||||
}
|
||||
#endif
|
||||
|
||||
SafePointScalarObjectNode* PhaseMacroExpand::create_scalarized_object_description(AllocateNode *alloc, SafePointNode* sfpt) {
|
||||
// Fields of scalar objs are referenced only at the end
|
||||
// of regular debuginfo at the last (youngest) JVMS.
|
||||
@ -848,6 +888,7 @@ SafePointScalarObjectNode* PhaseMacroExpand::create_scalarized_object_descriptio
|
||||
field_val = transform_later(new DecodeNNode(field_val, field_val->get_ptr_type()));
|
||||
}
|
||||
}
|
||||
DEBUG_ONLY(verify_type_compatability(field_val->bottom_type(), field_type);)
|
||||
sfpt->add_req(field_val);
|
||||
}
|
||||
|
||||
|
||||
@ -1377,6 +1377,9 @@ void Deoptimization::reassign_type_array_elements(frame* fr, RegisterMap* reg_ma
|
||||
|
||||
case T_INT: case T_FLOAT: { // 4 bytes.
|
||||
assert(value->type() == T_INT, "Agreement.");
|
||||
#if INCLUDE_JVMCI
|
||||
// big_value allows encoding double/long value as e.g. [int = 0, long], and storing
|
||||
// the value in two array elements.
|
||||
bool big_value = false;
|
||||
if (i + 1 < sv->field_size() && type == T_INT) {
|
||||
if (sv->field_at(i)->is_location()) {
|
||||
@ -1404,6 +1407,9 @@ void Deoptimization::reassign_type_array_elements(frame* fr, RegisterMap* reg_ma
|
||||
} else {
|
||||
obj->int_at_put(index, value->get_jint());
|
||||
}
|
||||
#else // not INCLUDE_JVMCI
|
||||
obj->int_at_put(index, value->get_jint());
|
||||
#endif // INCLUDE_JVMCI
|
||||
break;
|
||||
}
|
||||
|
||||
|
||||
@ -0,0 +1,123 @@
|
||||
/*
|
||||
* Copyright (c) 2025, 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.
|
||||
*/
|
||||
|
||||
package compiler.c2;
|
||||
|
||||
/*
|
||||
* @test
|
||||
* @bug 8370405
|
||||
* @summary Test case where we had escape analysis tell us that we can possibly eliminate
|
||||
* the array allocation, then MergeStores introduces a mismatched store, which
|
||||
* the actual elimination does not verify for. That led to wrong results.
|
||||
* @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.TestMergeStoresAndAllocationElimination::test
|
||||
* -XX:CompileCommand=exclude,compiler.c2.TestMergeStoresAndAllocationElimination::dontinline
|
||||
* -XX:-TieredCompilation -Xbatch
|
||||
* -XX:+IgnoreUnrecognizedVMOptions -XX:-CICompileOSR
|
||||
* compiler.c2.TestMergeStoresAndAllocationElimination
|
||||
* @run main compiler.c2.TestMergeStoresAndAllocationElimination
|
||||
*/
|
||||
|
||||
public class TestMergeStoresAndAllocationElimination {
|
||||
static void dontinline() {}
|
||||
|
||||
static int test(boolean flag) {
|
||||
int[] arr = new int[4];
|
||||
// The values below will be caputured as "raw stores" in the Initialize
|
||||
// of the array allocation above.
|
||||
// These stores are for cosmetics only, we set the "1" bits so that it is
|
||||
// simple to track where values are coming from.
|
||||
arr[0] = 0x0001_0000;
|
||||
arr[1] = 0x0010_0000;
|
||||
arr[2] = 0x0000_0100;
|
||||
arr[3] = 0x0100_0000;
|
||||
// So far, the result should be:
|
||||
// 0x421_0300
|
||||
|
||||
// The call below prevents further assignments from being captured into
|
||||
// the Initialize above.
|
||||
dontinline();
|
||||
// The follwoing stores are eventually optimized by MergeStores, and create
|
||||
// a mismatched StoreL.
|
||||
arr[0] = 0x0000_0001;
|
||||
arr[1] = 0x0000_0010;
|
||||
// Now, the result should be:
|
||||
// 0x400_0321
|
||||
|
||||
// We create an uncommon trap because of an "unstable if".
|
||||
// If Escape Analysis were to work, it would try to capture the values
|
||||
// from the StoreL above. But because it is mismatched, it should fail.
|
||||
// What happened before that verification: we would take the ConL, and
|
||||
// insert it in a list of ConI. That meant that we eventually applied
|
||||
// that value wrong if the deopt was taken (flag = true).
|
||||
//
|
||||
// What happened when the deopt got the wrong values: It got these values:
|
||||
// [0]=68719476737 = 0x10_0000_0001 -> long value, not correct
|
||||
// [1]=1048576 = 0x10_0000 -> this entry is not updated!
|
||||
// [2]=256 = 0x100
|
||||
// [3]=16777216 = 0x100_0000
|
||||
//
|
||||
// This is serialized as a long and 3 ints, and that looks like 5 ints.
|
||||
// This creates an array of 5 elements (and not 4):
|
||||
// [0] = 0x1
|
||||
// [1] = 0x10
|
||||
// [2] = 0x10_0000 -> this entry is "inserted"
|
||||
// [3] = 0x100
|
||||
// [4] = 0x100_0000
|
||||
//
|
||||
// This creates the wrong state:
|
||||
// 0x30_0421
|
||||
// And we can actually read that the arr.length is 5, below.
|
||||
if (flag) { System.out.println("unstable if: " + arr.length); }
|
||||
|
||||
// Delay the allocation elimination until after loop opts, so that it
|
||||
// happens after MergeStores. Without this, we would immediately
|
||||
// eliminate the allocation during Escape Analysis, and then MergeStores
|
||||
// would not find the stores that would be removed with the allocation.
|
||||
for (int i = 0; i < 10_000; i++) {
|
||||
arr[3] = 0x0000_1000;
|
||||
}
|
||||
// Coming from the correct value, we should have transition of state:
|
||||
// 0x400_0321 -> 0x4321
|
||||
// But coming from the bad (rematerialized) state, we transition:
|
||||
// 0x30_0421 -> 0x30_4021
|
||||
|
||||
// Tag each entry with an index number
|
||||
// We expect: 0x4321
|
||||
return 1 * arr[0] + 2 * arr[1] + 3 * arr[2] + 4 * arr[3];
|
||||
}
|
||||
|
||||
public static void main(String[] args) {
|
||||
// Capture interpreter result.
|
||||
int gold = test(false);
|
||||
// Repeat until we get compilation.
|
||||
for (int i = 0; i < 10_000; i++) {
|
||||
test(false);
|
||||
}
|
||||
// Capture compiled results.
|
||||
int res0 = test(false);
|
||||
int res1 = test(true);
|
||||
if (res0 != gold || res1 != gold) {
|
||||
throw new RuntimeException("Unexpected result: " + Integer.toHexString(res0) + " and " + Integer.toHexString(res1) + ", should be: " + Integer.toHexString(gold));
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,195 @@
|
||||
/*
|
||||
* Copyright (c) 2024, 2025, 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 id=yEA
|
||||
* @bug 8370405
|
||||
* @summary Test elimination of array allocation, and the rematerialization.
|
||||
* @library /test/lib /
|
||||
* @run driver compiler.escapeAnalysis.TestRematerializeObjects yEA
|
||||
*/
|
||||
|
||||
/*
|
||||
* @test id=nEA
|
||||
* @library /test/lib /
|
||||
* @run driver compiler.escapeAnalysis.TestRematerializeObjects nEA
|
||||
*/
|
||||
|
||||
package compiler.escapeAnalysis;
|
||||
|
||||
import jdk.test.lib.Utils;
|
||||
|
||||
import compiler.lib.ir_framework.*;
|
||||
import compiler.lib.verify.*;
|
||||
|
||||
public class TestRematerializeObjects {
|
||||
|
||||
public static void main(String[] args) {
|
||||
TestFramework framework = new TestFramework(TestRematerializeObjects.class);
|
||||
switch (args[0]) {
|
||||
case "yEA" -> { framework.addFlags("-XX:+EliminateAllocations"); }
|
||||
case "nEA" -> { framework.addFlags("-XX:-EliminateAllocations"); }
|
||||
default -> { throw new RuntimeException("Test argument not recognized: " + args[0]); }
|
||||
};
|
||||
framework.start();
|
||||
}
|
||||
|
||||
@DontInline
|
||||
static void dontinline() {}
|
||||
|
||||
@Run(test = "test1", mode = RunMode.STANDALONE)
|
||||
public void runTest1() {
|
||||
// Capture interpreter result.
|
||||
int gold = test1(false);
|
||||
// Repeat until we get compilation.
|
||||
for (int i = 0; i < 10_000; i++) {
|
||||
test1(false);
|
||||
}
|
||||
// Capture compiled results.
|
||||
int res0 = test1(false);
|
||||
int res1 = test1(true);
|
||||
if (res0 != gold || res1 != gold) {
|
||||
throw new RuntimeException("Unexpected result: " + Integer.toHexString(res0) + " and " +
|
||||
Integer.toHexString(res1) + ", should be: " + Integer.toHexString(gold));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@IR(counts = {IRNode.ALLOC_ARRAY, "1",
|
||||
IRNode.UNSTABLE_IF_TRAP, "1",
|
||||
IRNode.STORE_L_OF_CLASS, "int\\[int:4\\]", "1",
|
||||
IRNode.SAFEPOINT_SCALAROBJECT_OF, "fields@\\[0..3\\]", "0"},
|
||||
applyIf = {"EliminateAllocations", "false"})
|
||||
@IR(counts = {IRNode.ALLOC_ARRAY, "0",
|
||||
IRNode.UNSTABLE_IF_TRAP, "1",
|
||||
IRNode.STORE_L_OF_CLASS, "int\\[int:4\\]", "0",
|
||||
IRNode.SAFEPOINT_SCALAROBJECT_OF, "fields@\\[0..3\\]", "2"},
|
||||
applyIf = {"EliminateAllocations", "true"})
|
||||
static int test1(boolean flag) {
|
||||
int[] arr = new int[4];
|
||||
arr[0] = 0x0001_0000; // these slip into Initialize
|
||||
arr[1] = 0x0010_0000;
|
||||
arr[2] = 0x0000_0100;
|
||||
arr[3] = 0x0100_0000;
|
||||
dontinline();
|
||||
arr[0] = 0x0000_0001; // MergeStores -> StoreL
|
||||
arr[1] = 0x0000_0010;
|
||||
if (flag) {
|
||||
// unstable if -> deopt -> rematerialized array (if was eliminated)
|
||||
System.out.println("unstable if: " + arr.length);
|
||||
}
|
||||
arr[3] = 0x0000_1000;
|
||||
return 1 * arr[0] + 2 * arr[1] + 3 * arr[2] + 4 * arr[3];
|
||||
}
|
||||
|
||||
@Run(test = "test2", mode = RunMode.STANDALONE)
|
||||
public void runTest2() {
|
||||
// Capture interpreter result.
|
||||
int gold = test2(false);
|
||||
// Repeat until we get compilation.
|
||||
for (int i = 0; i < 10_000; i++) {
|
||||
test2(false);
|
||||
}
|
||||
// Capture compiled results.
|
||||
int res0 = test2(false);
|
||||
int res1 = test2(true);
|
||||
if (res0 != gold || res1 != gold) {
|
||||
throw new RuntimeException("Unexpected result: " + Integer.toHexString(res0) + " and " +
|
||||
Integer.toHexString(res1) + ", should be: " + Integer.toHexString(gold));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@IR(counts = {IRNode.ALLOC_ARRAY, "1",
|
||||
IRNode.UNSTABLE_IF_TRAP, "1",
|
||||
IRNode.STORE_I_OF_CLASS, "short\\[int:4\\]", "1",
|
||||
IRNode.SAFEPOINT_SCALAROBJECT_OF, "fields@\\[0..3\\]", "0"},
|
||||
applyIf = {"EliminateAllocations", "false"})
|
||||
@IR(counts = {IRNode.ALLOC_ARRAY, "0",
|
||||
IRNode.UNSTABLE_IF_TRAP, "1",
|
||||
IRNode.STORE_I_OF_CLASS, "short\\[int:4\\]", "0",
|
||||
IRNode.SAFEPOINT_SCALAROBJECT_OF, "fields@\\[0..3\\]", "2"},
|
||||
applyIf = {"EliminateAllocations", "true"})
|
||||
static int test2(boolean flag) {
|
||||
short[] arr = new short[4];
|
||||
arr[0] = 1;
|
||||
arr[1] = 2;
|
||||
arr[2] = 4;
|
||||
arr[3] = 8;
|
||||
dontinline();
|
||||
// Seems we detect that this is a short value passed into the short field.
|
||||
arr[0] = 16;
|
||||
arr[1] = 32;
|
||||
if (flag) {
|
||||
// unstable if -> deopt -> rematerialized array (if was eliminated)
|
||||
System.out.println("unstable if: " + arr.length);
|
||||
}
|
||||
arr[3] = 64;
|
||||
return 0x1 * arr[0] + 0x100 * arr[1] + 0x1_0000 * arr[2] + 0x100_0000 * arr[3];
|
||||
}
|
||||
|
||||
@Run(test = "test3", mode = RunMode.STANDALONE)
|
||||
public void runTest3() {
|
||||
// Capture interpreter result.
|
||||
int gold = test3(false, 42);
|
||||
// Repeat until we get compilation.
|
||||
for (int i = 0; i < 10_000; i++) {
|
||||
test3(false, 42);
|
||||
}
|
||||
// Capture compiled results.
|
||||
int res0 = test3(false, 42);
|
||||
int res1 = test3(true, 42);
|
||||
if (res0 != gold || res1 != gold) {
|
||||
throw new RuntimeException("Unexpected result: " + Integer.toHexString(res0) + " and " +
|
||||
Integer.toHexString(res1) + ", should be: " + Integer.toHexString(gold));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@IR(counts = {IRNode.ALLOC_ARRAY, "1",
|
||||
IRNode.UNSTABLE_IF_TRAP, "1",
|
||||
IRNode.SAFEPOINT_SCALAROBJECT_OF, "fields@\\[0..3\\]", "0"},
|
||||
applyIf = {"EliminateAllocations", "false"})
|
||||
@IR(counts = {IRNode.ALLOC_ARRAY, "0",
|
||||
IRNode.UNSTABLE_IF_TRAP, "1",
|
||||
IRNode.SAFEPOINT_SCALAROBJECT_OF, "fields@\\[0..3\\]", "2"},
|
||||
applyIf = {"EliminateAllocations", "true"})
|
||||
static int test3(boolean flag, int x) {
|
||||
short[] arr = new short[4];
|
||||
arr[0] = 1;
|
||||
arr[1] = 2;
|
||||
arr[2] = 4;
|
||||
arr[3] = 8;
|
||||
dontinline();
|
||||
// Here, we don't get ConI, but instead AddI, which means we are
|
||||
// serializing an int value, for a short slot.
|
||||
arr[0] = (short)(x + 1);
|
||||
arr[1] = (short)(x + 2);
|
||||
if (flag) {
|
||||
// unstable if -> deopt -> rematerialized array (if was eliminated)
|
||||
System.out.println("unstable if: " + arr.length);
|
||||
}
|
||||
arr[3] = 64;
|
||||
return 0x1 * arr[0] + 0x100 * arr[1] + 0x1_0000 * arr[2] + 0x100_0000 * arr[3];
|
||||
}
|
||||
}
|
||||
@ -1904,6 +1904,11 @@ public class IRNode {
|
||||
optoOnly(SCOPE_OBJECT, regex);
|
||||
}
|
||||
|
||||
public static final String SAFEPOINT_SCALAROBJECT_OF = COMPOSITE_PREFIX + "SAFEPOINT_SCALAROBJECT_OF" + POSTFIX;
|
||||
static {
|
||||
safepointScalarobjectOfNodes(SAFEPOINT_SCALAROBJECT_OF, "SafePointScalarObject");
|
||||
}
|
||||
|
||||
public static final String SIGNUM_VD = VECTOR_PREFIX + "SIGNUM_VD" + POSTFIX;
|
||||
static {
|
||||
vectorNode(SIGNUM_VD, "SignumVD", TYPE_DOUBLE);
|
||||
@ -3214,6 +3219,11 @@ public class IRNode {
|
||||
beforeMatching(irNodePlaceholder, regex);
|
||||
}
|
||||
|
||||
private static void safepointScalarobjectOfNodes(String irNodePlaceholder, String irNodeRegex) {
|
||||
String regex = START + irNodeRegex + MID + ".*" + IS_REPLACED + ".*" + END;
|
||||
beforeMatching(irNodePlaceholder, regex);
|
||||
}
|
||||
|
||||
private static void fromBeforeRemoveUselessToFinalCode(String irNodePlaceholder, String irNodeRegex) {
|
||||
String regex = START + irNodeRegex + MID + END;
|
||||
IR_NODE_MAPPINGS.put(irNodePlaceholder, new SinglePhaseRangeEntry(CompilePhase.PRINT_IDEAL, regex,
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user