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

[Philip Reames via llvm-commits]

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?


Repository:
  rL LLVM

http://reviews.llvm.org/D22001