[InstCombine] Remove weaker fence adjacent to a stronger fence

[anna]

Summary

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."

[lebedev.ri]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2470–2492

Would it go past 80col if Next is inlined?

[reames]

General idea seems reasonable to me, a few code comments.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2472

Stale comment.

2475

I believe the second check should subsume the identicalto one here.

llvm/test/Transforms/InstCombine/consecutive-fences.ll

15

Ah! A case you need to be careful of and I think the current code is wrong on.

The stronger fence must also have a stronger scope. For the moment, you could restrict the transform to the case where both fences are global scoped if desired.

We can't use a singlethread fence seq_cst to remove a global release fence.

[anna]

fix based on Philip's comments about scopes.

llvm/test/Transforms/InstCombine/consecutive-fences.ll

15

Thanks, I hadn't realized the part about scopes! Will fix the code.

[anna]

addressed review comments. Added extra tests.

[reames]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2471–2492

As I said before, I don't believe this check serves any purpose after the addition of the new transform.

2484

Style suggestion: pull out a isStrongerFence(FI1,FI2) helper which does both scope and ordering check, then call it twice. I believe the resulting code should be easier to read. Can be a lambda or static function as you prefer.

[anna]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2471–2492

Actually I intentionally left it in the updated code. It will consider fences with the same syncscopes as well (SyncScope::singleThread, "target-dependent" scope) for removal. I bail out for non-global syncscopes in the new transform.

I'm not sure what the "target-depdenent scope" is, but in the LangRef: "If an atomic operation is marked syncscope("<target-scope>"), where <target-scope> is a target specific synchronization scope, then it is target dependent if it synchronizes with and participates in the seq_cst total orderings of other operations." So, I am a bit iffy, if we can drop weaker fences in such scopes :)

2484

Yup, agreed.

[anna]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2471–2492

If we ignore "target dependent scopes" same ordered fence, then this check can be ignored, since I can update the new transform to handle both global and single-thread scopes.

[reames]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2471–2492

Ah, that concerns make sense. I don't really know the answer to the target scope question, so let's be conservative as well.

Please add a test with an arbitrary target scope, and a comment explicitly asking whether it's a missing transform. (i.e. give the next person who tries to remove this code a clue to hang their hat on)

[anna]

support system and singlethread syncscopes. Added test for target-dependent syncscope to showcase need for isIdenticalTo check.

[reames]

LGTM

[anna]

I spend sometime trying to see if the failing premerge tests seem related or maybe false positives, but I have no idea where these are from. Also, other completed unrelated review show the same failures: https://reviews.llvm.org/D94928

[reames]

In D118607#3287556, @anna wrote:

I spend sometime trying to see if the failing premerge tests seem related or maybe false positives, but I have no idea where these are from. Also, other completed unrelated review show the same failures: https://reviews.llvm.org/D94928

They look unrelated. The precommit test applies the patch to a randomly chosen base revision. If that revision happens to be bad, well...

(I generally ignore them tbh. Bitten me once, but only once so far.)

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

   }
   }
   // Some intrinsics (like experimental_gc_statepoint) can be used in invoke
   // context, so it is handled in visitCallBase and we should trigger it.
   return visitCallBase(*II);
 }
 
 // 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;
 }
 
 // InvokeInst simplification
 Instruction *InstCombinerImpl::visitInvokeInst(InvokeInst &II) {
   return visitCallBase(II);
 }

llvm/test/Transforms/InstCombine/consecutive-fences.ll

 ; RUN: opt -instcombine -S %s | FileCheck %s
 
 ; Make sure we collapse the fences in this case
 
 ; 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: }
 
 define void @tinkywinky() {
   fence seq_cst
   fence seq_cst
   fence seq_cst
   fence syncscope("singlethread") acquire
   fence syncscope("singlethread") acquire
   fence syncscope("singlethread") acquire
   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
 ; CHECK-NEXT:   ret void
 ; CHECK-NEXT: }
 
 define void @dipsy() {
   fence seq_cst
   fence syncscope("singlethread") seq_cst
   ret void
 }
 
 ; 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: }
 
 define void @patatino() {
   fence acquire
   fence seq_cst
   fence acquire
   fence seq_cst
   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
 ; CHECK: fence seq_cst
 define void @debug() {
   fence seq_cst
   tail call void @llvm.dbg.value(metadata i32 5, metadata !1, metadata !DIExpression()), !dbg !9
   fence seq_cst