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

[Anna Thomas via llvm-commits]

anna marked an inline comment as done.
anna added a comment.

In http://reviews.llvm.org/D22001#474590, @dexonsmith wrote:

> I don't see a patch on the list.  Is this a problem with how you created the Phab review?


Sent patch separately to @dexonsmith. Response on patch:
`isa` works on references as well as pointers, so this can just be

  if (isa<FenceInst>(*BBI))

Agree with Duncan, but the llvm ref states that this implicit conversion comes at a cost, and at some point in time, may be reverted to mean what standard iterators are. So, I'll leave it as-is to avoid any fix-ups required in future.

"Unfortunately, these implicit conversions come at a cost; they prevent these iterators from conforming to standard iterator conventions, and thus from being usable with standard algorithms and containers. Because of this, these implicit conversions may be removed some day, and operator* changed to return a pointer instead of a reference."
http://llvm.org/docs/ProgrammersManual.html#basic-inspection-and-traversal-routines


================
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
----------------
reames wrote:
> 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. 
That's right, updated the comment to say exactly why we do not care about the fence instruction (i.e. we can remove the fence, but we cannot currently remove stores that have ordering constraints). 

================
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
----------------
reames wrote:
> 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?
No, this test is from an existing test which already contains the `fence`. The only change is the addition of the `byval` attribute. 
In `handleEndBlock`, 3 kinds of store locations are considered while traversing the basic block:
1. stack allocated locations
2. attributes passed `byval` -- there's a check in the handleEndBlock.  
```if (AI.hasByValOrInAllocaAttr()) DeadStackObjects.insert(&AI);```
3. locations generated by calloc-like statements and are not captured.

 


Repository:
  rL LLVM

http://reviews.llvm.org/D22001