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

[Anna Thomas via Phabricator via llvm-commits]

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);
----------------
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 :) 



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2486
+
+  if (NFI && isSystemScopeFence(NFI) &&
+      isStrongerThan(NFI->getOrdering(), FI.getOrdering()))
----------------
reames wrote:
> 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.
Yup, agreed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118607