changeset 51008:8df91a1b549b

8206163: AArch64: incorrect code generation for StoreCM Summary: StoreCM may require planting a StoreStore barrier Reviewed-by: aph, zyao, roland
author adinn
date Mon, 09 Jul 2018 09:38:11 +0100
parents fc9dd181d70e
children 44b07bd68f6d
files src/hotspot/cpu/aarch64/aarch64.ad test/hotspot/jtreg/compiler/c2/aarch64/TestVolatiles.java
diffstat 2 files changed, 26 insertions(+), 60 deletions(-) [+]
line wrap: on
line diff
--- 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