From 74f24f335f5825ef114160520374625337253713 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 13 Dec 2021 10:28:48 -0800 Subject: [PATCH 1/4] Use ishst instead of ish --- src/coreclr/jit/codegen.h | 5 +++-- src/coreclr/jit/codegenarm64.cpp | 29 +++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 343bcb138300bb..e1c13beb0af139 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1429,8 +1429,9 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX enum BarrierKind { - BARRIER_FULL, // full barrier - BARRIER_LOAD_ONLY, // load barier + BARRIER_FULL, // full barrier + BARRIER_LOAD_ONLY, // load barier + BARRIER_STORE_ONLY, // store barrier }; void instGen_MemoryBarrier(BarrierKind barrierKind = BARRIER_FULL); diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index db45c98b68e63c..c9eec7ce40a42b 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -3280,7 +3280,7 @@ void CodeGen::genCodeForStoreInd(GenTreeStoreInd* tree) else { // issue a full memory barrier before a volatile StInd - instGen_MemoryBarrier(); + instGen_MemoryBarrier(BARRIER_STORE_ONLY); } } @@ -9396,30 +9396,43 @@ void CodeGen::instGen_MemoryBarrier(BarrierKind barrierKind) emitter::instrDesc* lastMemBarrier = GetEmitter()->emitLastMemBarrier; if ((lastMemBarrier != nullptr) && compiler->opts.OptimizationEnabled()) { - BarrierKind prevBarrierKind = BARRIER_FULL; + bool wasPrevBarrierLdOrStOnly = false; if (lastMemBarrier->idSmallCns() == INS_BARRIER_ISHLD) { - prevBarrierKind = BARRIER_LOAD_ONLY; + wasPrevBarrierLdOrStOnly = true; + } + else if (lastMemBarrier->idSmallCns() == INS_BARRIER_ISHST) + { + wasPrevBarrierLdOrStOnly = true; } else { - // Currently we only emit two kinds of barriers on arm64: + // Currently we only emit three kinds of barriers on arm64: // ISH - Full (inner shareable domain) // ISHLD - LoadOnly (inner shareable domain) + // ISHST - StoreOnly (inner shareable domain) assert(lastMemBarrier->idSmallCns() == INS_BARRIER_ISH); } - if ((prevBarrierKind == BARRIER_LOAD_ONLY) && (barrierKind == BARRIER_FULL)) + if (wasPrevBarrierLdOrStOnly && (barrierKind == BARRIER_FULL)) { - // Previous memory barrier: load-only, current: full + // Previous memory barrier: load-only or store-only, current: full // Upgrade the previous one to full - assert((prevBarrierKind == BARRIER_LOAD_ONLY) && (barrierKind == BARRIER_FULL)); lastMemBarrier->idSmallCns(INS_BARRIER_ISH); } } else { - GetEmitter()->emitIns_BARR(INS_dmb, barrierKind == BARRIER_LOAD_ONLY ? INS_BARRIER_ISHLD : INS_BARRIER_ISH); + insBarrier ins = INS_BARRIER_ISH; + if (barrierKind == BARRIER_LOAD_ONLY) + { + ins = INS_BARRIER_ISHLD; + } + else if (barrierKind == BARRIER_STORE_ONLY) + { + ins = INS_BARRIER_ISHST; + } + GetEmitter()->emitIns_BARR(INS_dmb, ins); } } From d2d6d7030f8ff6b4769f4536e7562cd3bb43abcf Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Wed, 15 Dec 2021 23:40:07 -0800 Subject: [PATCH 2/4] Do not contain address of volatile fields --- src/coreclr/jit/gentree.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 00c58d70bae1d2..a5f002d886f49a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -4083,6 +4083,12 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } } } + if (tree->gtFlags & GTF_IND_VOLATILE) + { + // For volatile store/loads when address is contained we always emit `dmb` + // if it's not - we emit stlr/ldlr + doAddrMode = false; + } if (doAddrMode && gtMarkAddrMode(addr, &costEx, &costSz, tree->TypeGet())) { goto DONE; From 687277b8385977f9912d2fa17db7e062492a4c77 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 16 Dec 2021 16:59:25 -0800 Subject: [PATCH 3/4] Do not contain address only for Arm64 --- src/coreclr/jit/gentree.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index a5f002d886f49a..6bc30ca68389c0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -4083,12 +4083,14 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } } } +#ifdef TARGET_ARM64 if (tree->gtFlags & GTF_IND_VOLATILE) { // For volatile store/loads when address is contained we always emit `dmb` - // if it's not - we emit stlr/ldlr + // if it's not - we emit one-way barriers i.e. ldar/stlr doAddrMode = false; } +#endif // TARGET_ARM64 if (doAddrMode && gtMarkAddrMode(addr, &costEx, &costSz, tree->TypeGet())) { goto DONE; From c9267a5e61ca51dd72bcd5cfb77e08d25de28422 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 4 Jan 2022 11:05:39 -0800 Subject: [PATCH 4/4] Remove ishst --- src/coreclr/jit/codegen.h | 5 ++--- src/coreclr/jit/codegenarm64.cpp | 32 +++++++++++--------------------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index e1c13beb0af139..343bcb138300bb 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1429,9 +1429,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX enum BarrierKind { - BARRIER_FULL, // full barrier - BARRIER_LOAD_ONLY, // load barier - BARRIER_STORE_ONLY, // store barrier + BARRIER_FULL, // full barrier + BARRIER_LOAD_ONLY, // load barier }; void instGen_MemoryBarrier(BarrierKind barrierKind = BARRIER_FULL); diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index c9eec7ce40a42b..9cb1620968c627 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -3280,7 +3280,10 @@ void CodeGen::genCodeForStoreInd(GenTreeStoreInd* tree) else { // issue a full memory barrier before a volatile StInd - instGen_MemoryBarrier(BARRIER_STORE_ONLY); + // Note: We cannot issue store barrier ishst because it is a weaker barrier. + // The loads can get rearranged around the barrier causing to read wrong + // value. + instGen_MemoryBarrier(); } } @@ -9396,43 +9399,30 @@ void CodeGen::instGen_MemoryBarrier(BarrierKind barrierKind) emitter::instrDesc* lastMemBarrier = GetEmitter()->emitLastMemBarrier; if ((lastMemBarrier != nullptr) && compiler->opts.OptimizationEnabled()) { - bool wasPrevBarrierLdOrStOnly = false; + BarrierKind prevBarrierKind = BARRIER_FULL; if (lastMemBarrier->idSmallCns() == INS_BARRIER_ISHLD) { - wasPrevBarrierLdOrStOnly = true; - } - else if (lastMemBarrier->idSmallCns() == INS_BARRIER_ISHST) - { - wasPrevBarrierLdOrStOnly = true; + prevBarrierKind = BARRIER_LOAD_ONLY; } else { - // Currently we only emit three kinds of barriers on arm64: + // Currently we only emit two kinds of barriers on arm64: // ISH - Full (inner shareable domain) // ISHLD - LoadOnly (inner shareable domain) - // ISHST - StoreOnly (inner shareable domain) assert(lastMemBarrier->idSmallCns() == INS_BARRIER_ISH); } - if (wasPrevBarrierLdOrStOnly && (barrierKind == BARRIER_FULL)) + if ((prevBarrierKind == BARRIER_LOAD_ONLY) && (barrierKind == BARRIER_FULL)) { - // Previous memory barrier: load-only or store-only, current: full + // Previous memory barrier: load-only, current: full // Upgrade the previous one to full + assert((prevBarrierKind == BARRIER_LOAD_ONLY) && (barrierKind == BARRIER_FULL)); lastMemBarrier->idSmallCns(INS_BARRIER_ISH); } } else { - insBarrier ins = INS_BARRIER_ISH; - if (barrierKind == BARRIER_LOAD_ONLY) - { - ins = INS_BARRIER_ISHLD; - } - else if (barrierKind == BARRIER_STORE_ONLY) - { - ins = INS_BARRIER_ISHST; - } - GetEmitter()->emitIns_BARR(INS_dmb, ins); + GetEmitter()->emitIns_BARR(INS_dmb, barrierKind == BARRIER_LOAD_ONLY ? INS_BARRIER_ISHLD : INS_BARRIER_ISH); } }