# HG changeset patch # User adinn # Date 1531125491 -3600 # Node ID 8df91a1b549b0ecb6138cb4e37128a3c77c41321 # Parent fc9dd181d70e61338eb8681205fee63cbef49275 8206163: AArch64: incorrect code generation for StoreCM Summary: StoreCM may require planting a StoreStore barrier Reviewed-by: aph, zyao, roland diff -r fc9dd181d70e -r 8df91a1b549b src/hotspot/cpu/aarch64/aarch64.ad --- 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; } diff -r fc9dd181d70e -r 8df91a1b549b test/hotspot/jtreg/compiler/c2/aarch64/TestVolatiles.java --- 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 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