[PATCH] D22001: [DSE] Remove dead stores in end blocks containing fence

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 5 13:41:26 PDT 2016

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Comments inline.  Generally looks correct, but needs a revision before I'm comfortable signing off.  Also, added JF as a reviewer since he's usually interested in memory model optimizations.

Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:695
@@ -694,1 +694,3 @@
+    // We can remove the dead stores in the end basic block, irrespective of the
+    // fence and its ordering (release/acquire/seq_cst). We only need to check
Stylistically, this code should be further down.  Just after the call site handling would be reasonable.

Comment wise, the explanation is a bit unclear.  The key point is that a fence doesn't make a memory location visible to any other thread.  It simply ensures ordering of already visible writes.  Thus, skipping over the fence doesn't change whether a store is dead or not.  I didn't get that from your comment. 

Comment at: test/Transforms/DeadStoreElimination/fence.ll:52
@@ +51,3 @@
+; The store to %addr.i can be removed since it is a byval attribute
+define void @test3(i32* byval %addr.i) {
+; CHECK-LABEL: @test3
I'm not familiar with the details of byval parameters.  Can you confirm this was copied from an existing test and the only change is adding the fence?



More information about the llvm-commits mailing list