[llvm] 4fc52db - [InstCombine] Remove weaker fence adjacent to a stronger fence

[Anna Thomas via llvm-commits]

Author: Anna Thomas
Date: 2022-02-01T11:05:34-08:00
New Revision: 4fc52db116033253ad5b078b317a9f7ae0db87d2

URL: https://github.com/llvm/llvm-project/commit/4fc52db116033253ad5b078b317a9f7ae0db87d2
DIFF: https://github.com/llvm/llvm-project/commit/4fc52db116033253ad5b078b317a9f7ae0db87d2.diff

LOG: [InstCombine] Remove weaker fence adjacent to a stronger fence

We have an instCombine rule to remove identical consecutive fences.
We can extend this to remove weaker fences when we have consecutive stronger
fence.

As stated in the LangRef, a fence with a stronger ordering also implies
ordering weaker than itself: "A fence which has seq_cst ordering, in addition to
having both acquire and release semantics specified above, participates in the
global program order of other seq_cst operations and/or fences."

Reviewed-By: reames

Differential Revision: https://reviews.llvm.org/D118607

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
    llvm/test/Transforms/InstCombine/consecutive-fences.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index 1fb46af46bee6..05b28328afbf3 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2468,10 +2468,28 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
 
 // Fence instruction simplification
 Instruction *InstCombinerImpl::visitFenceInst(FenceInst &FI) {
-  // Remove identical consecutive fences.
-  Instruction *Next = FI.getNextNonDebugInstruction();
-  if (auto *NFI = dyn_cast<FenceInst>(Next))
-    if (FI.isIdenticalTo(NFI))
+  auto *NFI = dyn_cast<FenceInst>(FI.getNextNonDebugInstruction());
+  // This check is solely here to handle arbitrary target-dependent syncscopes.
+  // TODO: Can remove if does not matter in practice.
+  if (NFI && FI.isIdenticalTo(NFI))
+    return eraseInstFromFunction(FI);
+
+  // Returns true if FI1 is identical or stronger fence than FI2.
+  auto isIdenticalOrStrongerFence = [](FenceInst *FI1, FenceInst *FI2) {
+    auto FI1SyncScope = FI1->getSyncScopeID();
+    // Consider same scope, where scope is global or single-thread.
+    if (FI1SyncScope != FI2->getSyncScopeID() ||
+        (FI1SyncScope != SyncScope::System &&
+         FI1SyncScope != SyncScope::SingleThread))
+      return false;
+
+    return isAtLeastOrStrongerThan(FI1->getOrdering(), FI2->getOrdering());
+  };
+  if (NFI && isIdenticalOrStrongerFence(NFI, &FI))
+    return eraseInstFromFunction(FI);
+
+  if (auto *PFI = dyn_cast_or_null<FenceInst>(FI.getPrevNonDebugInstruction()))
+    if (isIdenticalOrStrongerFence(PFI, &FI))
       return eraseInstFromFunction(FI);
   return nullptr;
 }

diff  --git a/llvm/test/Transforms/InstCombine/consecutive-fences.ll b/llvm/test/Transforms/InstCombine/consecutive-fences.ll
index 2b8a8e72a621c..73421f8d15afb 100644
--- a/llvm/test/Transforms/InstCombine/consecutive-fences.ll
+++ b/llvm/test/Transforms/InstCombine/consecutive-fences.ll
@@ -4,7 +4,7 @@
 
 ; CHECK-LABEL: define void @tinkywinky
 ; CHECK-NEXT:   fence seq_cst
-; CHECK-NEXT:   fence syncscope("singlethread") acquire
+; CHECK-NEXT:   fence syncscope("singlethread") acquire 
 ; CHECK-NEXT:   ret void
 ; CHECK-NEXT: }
 
@@ -18,6 +18,17 @@ define void @tinkywinky() {
   ret void
 }
 
+; Arbitrary target dependent scope
+; Is this transform really needed?
+; CHECK-LABEL: test_target_dependent_scope
+; CHECK-NEXT: fence syncscope("MSP430") acquire
+; CHECK-NEXT: ret void
+define void @test_target_dependent_scope() {
+  fence syncscope("MSP430") acquire
+  fence syncscope("MSP430") acquire
+  ret void
+}
+
 ; CHECK-LABEL: define void @dipsy
 ; CHECK-NEXT:   fence seq_cst
 ; CHECK-NEXT:   fence syncscope("singlethread") seq_cst
@@ -31,9 +42,6 @@ define void @dipsy() {
 }
 
 ; CHECK-LABEL: define void @patatino
-; CHECK-NEXT:   fence acquire
-; CHECK-NEXT:   fence seq_cst
-; CHECK-NEXT:   fence acquire
 ; CHECK-NEXT:   fence seq_cst
 ; CHECK-NEXT:   ret void
 ; CHECK-NEXT: }
@@ -46,6 +54,47 @@ define void @patatino() {
   ret void
 }
 
+; CHECK-LABEL: define void @weaker_fence_1
+; CHECK-NEXT: fence seq_cst
+; CHECK-NEXT: ret void
+define void @weaker_fence_1() {
+  fence seq_cst
+  fence release
+  fence seq_cst
+  ret void
+}
+
+; CHECK-LABEL: define void @weaker_fence_2
+; CHECK-NEXT: fence seq_cst
+; CHECK-NEXT: ret void
+define void @weaker_fence_2() {
+  fence seq_cst
+  fence release
+  fence seq_cst
+  fence acquire 
+  ret void
+}
+
+; Although acquire is a weaker ordering than seq_cst, it has a system scope,
+; compare to singlethread scope in seq_cst.
+; CHECK-LABEL: acquire_global_neg_test
+; CHECK-NEXT: fence acquire
+; CHECK-NEXT: fence syncscope("singlethread") seq_cst
+define void @acquire_global_neg_test() {
+  fence acquire 
+  fence acquire 
+  fence syncscope("singlethread") seq_cst 
+  ret void
+}
+
+; CHECK-LABEL: acquire_single_thread_scope
+; CHECK-NEXT: fence syncscope("singlethread") seq_cst 
+define void @acquire_single_thread_scope() {
+  fence syncscope("singlethread") acquire 
+  fence syncscope("singlethread") seq_cst 
+  ret void
+}
+
 ; CHECK-LABEL: define void @debug
 ; CHECK-NOT: fence
 ; CHECK: call void @llvm.dbg.value