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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2473
+  if (auto *NFI = dyn_cast<FenceInst>(Next)) {
+    // Remove identical consecutive fences.
+    // When we have a weaker-ordering fence consecutively followed by a
----------------
Stale comment.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2476
+    // stronger-ordering fence, we can remove the weaker one.
+    if (FI.isIdenticalTo(NFI) || isStrongerThan(NFI->getOrdering(), FI.getOrdering()))
+      return eraseInstFromFunction(FI);
----------------
I believe the second check should subsume the identicalto one here.


================
Comment at: llvm/test/Transforms/InstCombine/consecutive-fences.ll:14
   fence seq_cst
   fence syncscope("singlethread") acquire
   fence syncscope("singlethread") acquire
----------------
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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118607