8206163: AArch64: incorrect code generation for StoreCM
Summary: StoreCM may require planting a StoreStore barrier
Reviewed-by: aph, zyao, roland
--- a/src/hotspot/cpu/aarch64/aarch64.ad Mon Jul 09 10:19:51 2018 +0200
+++ b/src/hotspot/cpu/aarch64/aarch64.ad Mon Jul 09 09:38:11 2018 +0100
@@ -1471,7 +1471,7 @@
// Ctl+Mem to a StoreB node (which does the actual card mark).
//
// n.b. a StoreCM node will only appear in this configuration when
- // using CMS. StoreCM differs from a normal card mark write (StoreB)
+ // using CMS or G1. StoreCM differs from a normal card mark write (StoreB)
// because it implies a requirement to order visibility of the card
// mark (StoreCM) relative to the object put (StoreP/N) using a
// StoreStore memory barrier (arguably this ought to be represented
@@ -1481,16 +1481,12 @@
// the sequence
//
// dmb ishst
- // stlrb
- //
- // However, in the case of a volatile put if we can recognise this
- // configuration and plant an stlr for the object write then we can
- // omit the dmb and just plant an strb since visibility of the stlr
- // is ordered before visibility of subsequent stores. StoreCM nodes
- // also arise when using G1 or using CMS with conditional card
- // marking. In these cases (as we shall see) we don't need to insert
- // the dmb when translating StoreCM because there is already an
- // intervening StoreLoad barrier between it and the StoreP/N.
+ // strb
+ //
+ // However, when using G1 or CMS with conditional card marking (as
+ // we shall see) we don't need to insert the dmb when translating
+ // StoreCM because there is already an intervening StoreLoad barrier
+ // between it and the StoreP/N.
//
// It is also possible to perform the card mark conditionally on it
// currently being unmarked in which case the volatile put graph
@@ -2868,50 +2864,17 @@
{
assert(storecm->Opcode() == Op_StoreCM, "expecting a StoreCM");
- // we only ever need to generate a dmb ishst between an object put
- // and the associated card mark when we are using CMS without
- // conditional card marking
-
- if (!UseConcMarkSweepGC || UseCondCardMark) {
- return true;
- }
-
- // if we are implementing volatile puts using barriers then the
- // object put is an str so we must insert the dmb ishst
-
- if (UseBarriersForVolatile) {
+ // we need to generate a dmb ishst between an object put and the
+ // associated card mark when we are using CMS without conditional
+ // card marking
+
+ if (UseConcMarkSweepGC && !UseCondCardMark) {
return false;
}
- // we can omit the dmb ishst if this StoreCM is part of a volatile
- // put because in thta case the put will be implemented by stlr
- //
- // we need to check for a normal subgraph feeding this StoreCM.
- // that means the StoreCM must be fed Memory from a leading membar,
- // either a MemBarRelease or its dependent MemBarCPUOrder, and the
- // leading membar must be part of a normal subgraph
-
- Node *x = storecm->in(StoreNode::Memory);
-
- if (!x->is_Proj()) {
- return false;
- }
-
- x = x->in(0);
-
- if (!x->is_MemBar()) {
- return false;
- }
-
- MemBarNode *leading = x->as_MemBar();
-
- // reject invalid candidates
- if (!leading_membar(leading)) {
- return false;
- }
-
- // we can omit the StoreStore if it is the head of a normal subgraph
- return (leading_to_normal(leading) != NULL);
+ // a storestore is unnecesary in all other cases
+
+ return true;
}
--- a/test/hotspot/jtreg/compiler/c2/aarch64/TestVolatiles.java Mon Jul 09 10:19:51 2018 +0200
+++ b/test/hotspot/jtreg/compiler/c2/aarch64/TestVolatiles.java Mon Jul 09 09:38:11 2018 +0100
@@ -34,7 +34,7 @@
* TestUnsafeVolatileCAS}
* and <testtype> in {G1,
* CMS,
- * CMSCondCardMark,
+ * CMSCondMark,
* Serial,
* Parallel}
*/
@@ -287,7 +287,7 @@
"ret"
};
break;
- case "CMSCondCardMark":
+ case "CMSCondMark":
// a card mark volatile barrier should be generated
// before the card mark strb from the StoreCM and the
// storestore barrier from the StoreCM should be elided
@@ -305,11 +305,13 @@
case "CMS":
// a volatile card mark membar should not be generated
// before the card mark strb from the StoreCM and the
- // storestore barrier from the StoreCM should be elided
+ // storestore barrier from the StoreCM should be
+ // generated as "dmb ishst"
matches = new String[] {
"membar_release (elided)",
"stlrw",
- "storestore (elided)",
+ "storestore",
+ "dmb ishst",
"strb",
"membar_volatile (elided)",
"ret"
@@ -344,7 +346,7 @@
"ret"
};
break;
- case "CMSCondCardMark":
+ case "CMSCondMark":
// a card mark volatile barrier should be generated
// before the card mark strb from the StoreCM and the
// storestore barrier from the StoreCM should be elided
@@ -443,7 +445,7 @@
"ret"
};
break;
- case "CMSCondCardMark":
+ case "CMSCondMark":
// a card mark volatile barrier should be generated
// before the card mark strb from the StoreCM and the
// storestore barrier from the StoreCM should be elided
@@ -465,7 +467,8 @@
matches = new String[] {
"membar_release (elided)",
"cmpxchgw_acq",
- "storestore (elided)",
+ "storestore",
+ "dmb ishst",
"strb",
"membar_acquire (elided)",
"ret"
@@ -500,7 +503,7 @@
"ret"
};
break;
- case "CMSCondCardMark":
+ case "CMSCondMark":
// a card mark volatile barrier should be generated
// before the card mark strb from the StoreCM and the
// storestore barrier from the StoreCM should be elided