Skip to content

Commit 74fdd68

Browse files
committed
8333791: Fix memory barriers for @stable fields
Reviewed-by: liach, vlivanov
1 parent da7311b commit 74fdd68

File tree

14 files changed

+1047
-39
lines changed

14 files changed

+1047
-39
lines changed

src/hotspot/share/c1/c1_GraphBuilder.cpp

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,7 +1563,7 @@ void GraphBuilder::method_return(Value x, bool ignore_return) {
15631563
// The conditions for a memory barrier are described in Parse::do_exits().
15641564
bool need_mem_bar = false;
15651565
if (method()->name() == ciSymbols::object_initializer_name() &&
1566-
(scope()->wrote_final() ||
1566+
(scope()->wrote_final() || scope()->wrote_stable() ||
15671567
(AlwaysSafeConstructors && scope()->wrote_fields()) ||
15681568
(support_IRIW_for_not_multiple_copy_atomic_cpu && scope()->wrote_volatile()))) {
15691569
need_mem_bar = true;
@@ -1741,15 +1741,17 @@ void GraphBuilder::access_field(Bytecodes::Code code) {
17411741
}
17421742
}
17431743

1744-
if (field->is_final() && (code == Bytecodes::_putfield)) {
1745-
scope()->set_wrote_final();
1746-
}
1747-
17481744
if (code == Bytecodes::_putfield) {
17491745
scope()->set_wrote_fields();
17501746
if (field->is_volatile()) {
17511747
scope()->set_wrote_volatile();
17521748
}
1749+
if (field->is_final()) {
1750+
scope()->set_wrote_final();
1751+
}
1752+
if (field->is_stable()) {
1753+
scope()->set_wrote_stable();
1754+
}
17531755
}
17541756

17551757
const int offset = !needs_patching ? field->offset_in_bytes() : -1;

src/hotspot/share/c1/c1_IR.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ IRScope::IRScope(Compilation* compilation, IRScope* caller, int caller_bci, ciMe
146146
_wrote_final = false;
147147
_wrote_fields = false;
148148
_wrote_volatile = false;
149+
_wrote_stable = false;
149150
_start = nullptr;
150151

151152
if (osr_bci != -1) {

src/hotspot/share/c1/c1_IR.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ class IRScope: public CompilationResourceObj {
149149
bool _wrote_final; // has written final field
150150
bool _wrote_fields; // has written fields
151151
bool _wrote_volatile; // has written volatile field
152+
bool _wrote_stable; // has written @Stable field
152153
BlockBegin* _start; // the start block, successsors are method entries
153154

154155
ResourceBitMap _requires_phi_function; // bit is set if phi functions at loop headers are necessary for a local variable
@@ -187,6 +188,8 @@ class IRScope: public CompilationResourceObj {
187188
bool wrote_fields () const { return _wrote_fields; }
188189
void set_wrote_volatile() { _wrote_volatile = true; }
189190
bool wrote_volatile () const { return _wrote_volatile; }
191+
void set_wrote_stable() { _wrote_stable = true; }
192+
bool wrote_stable() const { return _wrote_stable; }
190193
};
191194

192195

src/hotspot/share/opto/parse.hpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ class Parse : public GraphKit {
358358
bool _wrote_volatile; // Did we write a volatile field?
359359
bool _wrote_stable; // Did we write a @Stable field?
360360
bool _wrote_fields; // Did we write any field?
361-
Node* _alloc_with_final; // An allocation node with final field
361+
Node* _alloc_with_final_or_stable; // An allocation node with final or @Stable field
362362

363363
// Variables which track Java semantics during bytecode parsing:
364364

@@ -403,10 +403,10 @@ class Parse : public GraphKit {
403403
void set_wrote_stable(bool z) { _wrote_stable = z; }
404404
bool wrote_fields() const { return _wrote_fields; }
405405
void set_wrote_fields(bool z) { _wrote_fields = z; }
406-
Node* alloc_with_final() const { return _alloc_with_final; }
407-
void set_alloc_with_final(Node* n) {
408-
assert((_alloc_with_final == nullptr) || (_alloc_with_final == n), "different init objects?");
409-
_alloc_with_final = n;
406+
Node* alloc_with_final_or_stable() const { return _alloc_with_final_or_stable; }
407+
void set_alloc_with_final_or_stable(Node* n) {
408+
assert((_alloc_with_final_or_stable == nullptr) || (_alloc_with_final_or_stable == n), "different init objects?");
409+
_alloc_with_final_or_stable = n;
410410
}
411411

412412
Block* block() const { return _block; }

src/hotspot/share/opto/parse1.cpp

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,7 @@ Parse::Parse(JVMState* caller, ciMethod* parse_method, float expected_uses)
412412
_wrote_volatile = false;
413413
_wrote_stable = false;
414414
_wrote_fields = false;
415-
_alloc_with_final = nullptr;
415+
_alloc_with_final_or_stable = nullptr;
416416
_block = nullptr;
417417
_first_return = true;
418418
_replaced_nodes_for_exceptions = false;
@@ -988,8 +988,8 @@ void Parse::do_exits() {
988988
// Figure out if we need to emit the trailing barrier. The barrier is only
989989
// needed in the constructors, and only in three cases:
990990
//
991-
// 1. The constructor wrote a final. The effects of all initializations
992-
// must be committed to memory before any code after the constructor
991+
// 1. The constructor wrote a final or a @Stable field. All these
992+
// initializations must be ordered before any code after the constructor
993993
// publishes the reference to the newly constructed object. Rather
994994
// than wait for the publication, we simply block the writes here.
995995
// Rather than put a barrier on only those writes which are required
@@ -1014,34 +1014,23 @@ void Parse::do_exits() {
10141014
// exceptional returns, since they cannot publish normally.
10151015
//
10161016
if (method()->is_object_initializer() &&
1017-
(wrote_final() ||
1017+
(wrote_final() || wrote_stable() ||
10181018
(AlwaysSafeConstructors && wrote_fields()) ||
10191019
(support_IRIW_for_not_multiple_copy_atomic_cpu && wrote_volatile()))) {
1020+
Node* recorded_alloc = alloc_with_final_or_stable();
10201021
_exits.insert_mem_bar(UseStoreStoreForCtor ? Op_MemBarStoreStore : Op_MemBarRelease,
1021-
alloc_with_final());
1022+
recorded_alloc);
10221023

10231024
// If Memory barrier is created for final fields write
10241025
// and allocation node does not escape the initialize method,
10251026
// then barrier introduced by allocation node can be removed.
1026-
if (DoEscapeAnalysis && alloc_with_final()) {
1027-
AllocateNode* alloc = AllocateNode::Ideal_allocation(alloc_with_final());
1027+
if (DoEscapeAnalysis && (recorded_alloc != nullptr)) {
1028+
AllocateNode* alloc = AllocateNode::Ideal_allocation(recorded_alloc);
10281029
alloc->compute_MemBar_redundancy(method());
10291030
}
10301031
if (PrintOpto && (Verbose || WizardMode)) {
10311032
method()->print_name();
1032-
tty->print_cr(" writes finals and needs a memory barrier");
1033-
}
1034-
}
1035-
1036-
// Any method can write a @Stable field; insert memory barriers
1037-
// after those also. Can't bind predecessor allocation node (if any)
1038-
// with barrier because allocation doesn't always dominate
1039-
// MemBarRelease.
1040-
if (wrote_stable()) {
1041-
_exits.insert_mem_bar(Op_MemBarRelease);
1042-
if (PrintOpto && (Verbose || WizardMode)) {
1043-
method()->print_name();
1044-
tty->print_cr(" writes @Stable and needs a memory barrier");
1033+
tty->print_cr(" writes finals/@Stable and needs a memory barrier");
10451034
}
10461035
}
10471036

src/hotspot/share/opto/parse3.cpp

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -236,22 +236,25 @@ void Parse::do_put_xxx(Node* obj, ciField* field, bool is_field) {
236236
set_wrote_fields(true);
237237

238238
// If the field is final, the rules of Java say we are in <init> or <clinit>.
239-
// Note the presence of writes to final non-static fields, so that we
239+
// If the field is @Stable, we can be in any method, but we only care about
240+
// constructors at this point.
241+
//
242+
// Note the presence of writes to final/@Stable non-static fields, so that we
240243
// can insert a memory barrier later on to keep the writes from floating
241244
// out of the constructor.
242-
// Any method can write a @Stable field; insert memory barriers after those also.
243-
if (field->is_final()) {
244-
set_wrote_final(true);
245+
if (field->is_final() || field->is_stable()) {
246+
if (field->is_final()) {
247+
set_wrote_final(true);
248+
}
249+
if (field->is_stable()) {
250+
set_wrote_stable(true);
251+
}
245252
if (AllocateNode::Ideal_allocation(obj) != nullptr) {
246253
// Preserve allocation ptr to create precedent edge to it in membar
247254
// generated on exit from constructor.
248-
// Can't bind stable with its allocation, only record allocation for final field.
249-
set_alloc_with_final(obj);
255+
set_alloc_with_final_or_stable(obj);
250256
}
251257
}
252-
if (field->is_stable()) {
253-
set_wrote_stable(true);
254-
}
255258
}
256259
}
257260

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/*
2+
* Copyright Amazon.com Inc. 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+
/*
25+
* @test
26+
* @bug 8333791
27+
* @requires os.arch=="aarch64" | os.arch=="riscv64" | os.arch=="x86_64" | os.arch=="amd64"
28+
* @requires vm.gc.Parallel
29+
* @requires vm.compiler2.enabled
30+
* @summary Check stable field folding and barriers
31+
* @modules java.base/jdk.internal.vm.annotation
32+
* @library /test/lib /
33+
* @run driver compiler.c2.irTests.stable.StablePrimArrayTest
34+
*/
35+
36+
package compiler.c2.irTests.stable;
37+
38+
import compiler.lib.ir_framework.*;
39+
import jdk.test.lib.Asserts;
40+
41+
import jdk.internal.vm.annotation.Stable;
42+
43+
public class StablePrimArrayTest {
44+
45+
public static void main(String[] args) {
46+
TestFramework tf = new TestFramework();
47+
tf.addTestClassesToBootClassPath();
48+
tf.addFlags(
49+
"-XX:+UnlockExperimentalVMOptions",
50+
"-XX:CompileThreshold=100",
51+
"-XX:-TieredCompilation",
52+
"-XX:+UseParallelGC"
53+
);
54+
tf.start();
55+
}
56+
57+
static final int[] EMPTY_INTEGER = new int[] { 0 };
58+
static final int[] FULL_INTEGER = new int[] { 42 };
59+
60+
static class Carrier {
61+
@Stable
62+
int[] field;
63+
64+
@ForceInline
65+
public Carrier(int initLevel) {
66+
switch (initLevel) {
67+
case 0:
68+
// Do nothing.
69+
break;
70+
case 1:
71+
field = EMPTY_INTEGER;
72+
break;
73+
case 2:
74+
field = FULL_INTEGER;
75+
break;
76+
default:
77+
throw new IllegalStateException("Unknown level");
78+
}
79+
}
80+
81+
@ForceInline
82+
public void initEmpty() {
83+
field = EMPTY_INTEGER;
84+
}
85+
86+
@ForceInline
87+
public void initFull() {
88+
field = FULL_INTEGER;
89+
}
90+
91+
}
92+
93+
static final Carrier BLANK_CARRIER = new Carrier(0);
94+
static final Carrier INIT_EMPTY_CARRIER = new Carrier(1);
95+
static final Carrier INIT_FULL_CARRIER = new Carrier(2);
96+
97+
@Test
98+
@IR(counts = { IRNode.LOAD, ">0" })
99+
@IR(failOn = { IRNode.MEMBAR })
100+
static int testNoFold() {
101+
// Access should not be folded.
102+
// No barriers expected for plain fields.
103+
int[] is = BLANK_CARRIER.field;
104+
if (is != null) {
105+
return is[0];
106+
}
107+
return 0;
108+
}
109+
110+
@Test
111+
@IR(counts = { IRNode.LOAD, ">0" })
112+
@IR(failOn = { IRNode.MEMBAR })
113+
static int testPartialFold() {
114+
// Access should not be folded.
115+
// No barriers expected for plain fields.
116+
int[] is = INIT_EMPTY_CARRIER.field;
117+
if (is != null) {
118+
return is[0];
119+
}
120+
return 0;
121+
}
122+
123+
124+
@Test
125+
@IR(failOn = { IRNode.LOAD, IRNode.MEMBAR })
126+
static int testFold() {
127+
// Access should be completely folded.
128+
int[] is = INIT_FULL_CARRIER.field;
129+
if (is != null) {
130+
return is[0];
131+
}
132+
return 0;
133+
}
134+
135+
@Test
136+
@IR(counts = { IRNode.MEMBAR_STORESTORE, "1" })
137+
static Carrier testConstructorBlankInit() {
138+
// Only the header barrier.
139+
return new Carrier(0);
140+
}
141+
142+
@Test
143+
@IR(counts = { IRNode.MEMBAR_STORESTORE, "1" })
144+
static Carrier testConstructorEmptyInit() {
145+
// Only the header barrier.
146+
return new Carrier(1);
147+
}
148+
149+
@Test
150+
@IR(counts = { IRNode.MEMBAR_STORESTORE, "1" })
151+
static Carrier testConstructorFullInit() {
152+
// Only the header barrier.
153+
return new Carrier(2);
154+
}
155+
156+
@Test
157+
@IR(failOn = { IRNode.MEMBAR })
158+
static void testMethodEmptyInit() {
159+
// Reference inits do not have membars.
160+
INIT_EMPTY_CARRIER.initEmpty();
161+
}
162+
163+
@Test
164+
@IR(failOn = { IRNode.MEMBAR })
165+
static void testMethodFullInit() {
166+
// Reference inits do not have membars.
167+
INIT_FULL_CARRIER.initFull();
168+
}
169+
170+
}

0 commit comments

Comments
 (0)