[PATCH] D118607: [InstCombine] Remove weaker fence adjacent to a stronger fence

Mon Jan 31 12:12:04 PST 2022

anna added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2473
   // Remove identical consecutive fences.
-  Instruction *Next = FI.getNextNonDebugInstruction();
-  if (auto *NFI = dyn_cast<FenceInst>(Next))
-    if (FI.isIdenticalTo(NFI))
+  if (NFI && FI.isIdenticalTo(NFI))
+    return eraseInstFromFunction(FI);
----------------
anna wrote:
> reames wrote:
> > As I said before, I don't believe this check serves any purpose after the addition of the new transform.  
> 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 :) 
> 
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118607/new/

https://reviews.llvm.org/D118607