Skip to content

Commit 09a047f

Browse files
eme64oliviermattmannmerykitty
committed
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
1 parent 0ca0852 commit 09a047f

File tree

5 files changed

+375
-0
lines changed

5 files changed

+375
-0
lines changed

src/hotspot/share/opto/macro.cpp

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,11 @@ bool PhaseMacroExpand::can_eliminate_allocation(PhaseIterGVN* igvn, AllocateNode
595595
for (DUIterator_Fast kmax, k = use->fast_outs(kmax);
596596
k < kmax && can_eliminate; k++) {
597597
Node* n = use->fast_out(k);
598+
if (n->is_Mem() && n->as_Mem()->is_mismatched_access()) {
599+
DEBUG_ONLY(disq_node = n);
600+
NOT_PRODUCT(fail_eliminate = "Mismatched access");
601+
can_eliminate = false;
602+
}
598603
if (!n->is_Store() && n->Opcode() != Op_CastP2X && !bs->is_gc_pre_barrier_node(n) && !reduce_merge_precheck) {
599604
DEBUG_ONLY(disq_node = n;)
600605
if (n->is_Load() || n->is_LoadStore()) {
@@ -732,6 +737,41 @@ void PhaseMacroExpand::undo_previous_scalarizations(GrowableArray <SafePointNode
732737
}
733738
}
734739

740+
#ifdef ASSERT
741+
// Verify if a value can be written into a field.
742+
void verify_type_compatability(const Type* value_type, const Type* field_type) {
743+
BasicType value_bt = value_type->basic_type();
744+
BasicType field_bt = field_type->basic_type();
745+
746+
// Primitive types must match.
747+
if (is_java_primitive(value_bt) && value_bt == field_bt) { return; }
748+
749+
// I have been struggling to make a similar assert for non-primitive
750+
// types. I we can add one in the future. For now, I just let them
751+
// pass without checks.
752+
// In particular, I was struggling with a value that came from a call,
753+
// and had only a non-null check CastPP. There was also a checkcast
754+
// in the graph to verify the interface, but the corresponding
755+
// CheckCastPP result was not updated in the stack slot, and so
756+
// we ended up using the CastPP. That means that the field knows
757+
// that it should get an oop from an interface, but the value lost
758+
// that information, and so it is not a subtype.
759+
// There may be other issues, feel free to investigate further!
760+
if (!is_java_primitive(value_bt)) { return; }
761+
762+
tty->print_cr("value not compatible for field: %s vs %s",
763+
type2name(value_bt),
764+
type2name(field_bt));
765+
tty->print("value_type: ");
766+
value_type->dump();
767+
tty->cr();
768+
tty->print("field_type: ");
769+
field_type->dump();
770+
tty->cr();
771+
assert(false, "value_type does not fit field_type");
772+
}
773+
#endif
774+
735775
SafePointScalarObjectNode* PhaseMacroExpand::create_scalarized_object_description(AllocateNode *alloc, SafePointNode* sfpt) {
736776
// Fields of scalar objs are referenced only at the end
737777
// of regular debuginfo at the last (youngest) JVMS.
@@ -848,6 +888,7 @@ SafePointScalarObjectNode* PhaseMacroExpand::create_scalarized_object_descriptio
848888
field_val = transform_later(new DecodeNNode(field_val, field_val->get_ptr_type()));
849889
}
850890
}
891+
DEBUG_ONLY(verify_type_compatability(field_val->bottom_type(), field_type);)
851892
sfpt->add_req(field_val);
852893
}
853894

src/hotspot/share/runtime/deoptimization.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1377,6 +1377,9 @@ void Deoptimization::reassign_type_array_elements(frame* fr, RegisterMap* reg_ma
13771377

13781378
case T_INT: case T_FLOAT: { // 4 bytes.
13791379
assert(value->type() == T_INT, "Agreement.");
1380+
#if INCLUDE_JVMCI
1381+
// big_value allows encoding double/long value as e.g. [int = 0, long], and storing
1382+
// the value in two array elements.
13801383
bool big_value = false;
13811384
if (i + 1 < sv->field_size() && type == T_INT) {
13821385
if (sv->field_at(i)->is_location()) {
@@ -1404,6 +1407,9 @@ void Deoptimization::reassign_type_array_elements(frame* fr, RegisterMap* reg_ma
14041407
} else {
14051408
obj->int_at_put(index, value->get_jint());
14061409
}
1410+
#else // not INCLUDE_JVMCI
1411+
obj->int_at_put(index, value->get_jint());
1412+
#endif // INCLUDE_JVMCI
14071413
break;
14081414
}
14091415

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/*
2+
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
package compiler.c2;
25+
26+
/*
27+
* @test
28+
* @bug 8370405
29+
* @summary Test case where we had escape analysis tell us that we can possibly eliminate
30+
* the array allocation, then MergeStores introduces a mismatched store, which
31+
* the actual elimination does not verify for. That led to wrong results.
32+
* @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.TestMergeStoresAndAllocationElimination::test
33+
* -XX:CompileCommand=exclude,compiler.c2.TestMergeStoresAndAllocationElimination::dontinline
34+
* -XX:-TieredCompilation -Xbatch
35+
* -XX:+IgnoreUnrecognizedVMOptions -XX:-CICompileOSR
36+
* compiler.c2.TestMergeStoresAndAllocationElimination
37+
* @run main compiler.c2.TestMergeStoresAndAllocationElimination
38+
*/
39+
40+
public class TestMergeStoresAndAllocationElimination {
41+
static void dontinline() {}
42+
43+
static int test(boolean flag) {
44+
int[] arr = new int[4];
45+
// The values below will be caputured as "raw stores" in the Initialize
46+
// of the array allocation above.
47+
// These stores are for cosmetics only, we set the "1" bits so that it is
48+
// simple to track where values are coming from.
49+
arr[0] = 0x0001_0000;
50+
arr[1] = 0x0010_0000;
51+
arr[2] = 0x0000_0100;
52+
arr[3] = 0x0100_0000;
53+
// So far, the result should be:
54+
// 0x421_0300
55+
56+
// The call below prevents further assignments from being captured into
57+
// the Initialize above.
58+
dontinline();
59+
// The follwoing stores are eventually optimized by MergeStores, and create
60+
// a mismatched StoreL.
61+
arr[0] = 0x0000_0001;
62+
arr[1] = 0x0000_0010;
63+
// Now, the result should be:
64+
// 0x400_0321
65+
66+
// We create an uncommon trap because of an "unstable if".
67+
// If Escape Analysis were to work, it would try to capture the values
68+
// from the StoreL above. But because it is mismatched, it should fail.
69+
// What happened before that verification: we would take the ConL, and
70+
// insert it in a list of ConI. That meant that we eventually applied
71+
// that value wrong if the deopt was taken (flag = true).
72+
//
73+
// What happened when the deopt got the wrong values: It got these values:
74+
// [0]=68719476737 = 0x10_0000_0001 -> long value, not correct
75+
// [1]=1048576 = 0x10_0000 -> this entry is not updated!
76+
// [2]=256 = 0x100
77+
// [3]=16777216 = 0x100_0000
78+
//
79+
// This is serialized as a long and 3 ints, and that looks like 5 ints.
80+
// This creates an array of 5 elements (and not 4):
81+
// [0] = 0x1
82+
// [1] = 0x10
83+
// [2] = 0x10_0000 -> this entry is "inserted"
84+
// [3] = 0x100
85+
// [4] = 0x100_0000
86+
//
87+
// This creates the wrong state:
88+
// 0x30_0421
89+
// And we can actually read that the arr.length is 5, below.
90+
if (flag) { System.out.println("unstable if: " + arr.length); }
91+
92+
// Delay the allocation elimination until after loop opts, so that it
93+
// happens after MergeStores. Without this, we would immediately
94+
// eliminate the allocation during Escape Analysis, and then MergeStores
95+
// would not find the stores that would be removed with the allocation.
96+
for (int i = 0; i < 10_000; i++) {
97+
arr[3] = 0x0000_1000;
98+
}
99+
// Coming from the correct value, we should have transition of state:
100+
// 0x400_0321 -> 0x4321
101+
// But coming from the bad (rematerialized) state, we transition:
102+
// 0x30_0421 -> 0x30_4021
103+
104+
// Tag each entry with an index number
105+
// We expect: 0x4321
106+
return 1 * arr[0] + 2 * arr[1] + 3 * arr[2] + 4 * arr[3];
107+
}
108+
109+
public static void main(String[] args) {
110+
// Capture interpreter result.
111+
int gold = test(false);
112+
// Repeat until we get compilation.
113+
for (int i = 0; i < 10_000; i++) {
114+
test(false);
115+
}
116+
// Capture compiled results.
117+
int res0 = test(false);
118+
int res1 = test(true);
119+
if (res0 != gold || res1 != gold) {
120+
throw new RuntimeException("Unexpected result: " + Integer.toHexString(res0) + " and " + Integer.toHexString(res1) + ", should be: " + Integer.toHexString(gold));
121+
}
122+
}
123+
}

0 commit comments

Comments
 (0)