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

[Philip Reames via Phabricator via llvm-commits]

reames 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:
> 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.
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118607