[PATCH] D11710: [DSE] Enable removal of lifetime intrinsics in terminating blocks

[Björn Steinbrink via llvm-commits]

dotdash added a comment.

The test with invoke was just to have an unwind block, because that's what my original testcase had. I'll remove that test because with just a call it adds nothing that isn't already handled in the other tests.

I'll keep the special casing for lifetime start/end for now given because I think I need it for lifetime start anyway and that keeps hasMemoryWrite and isRemovable simpler. If you think it's better to integrate the special case with those two functions though, I'll do that.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:739
@@ +738,3 @@
+    if (removeLifetimeEnds)
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI))
+        switch (II->getIntrinsicID()) {
----------------
reames wrote:
> More generally, the special casing here seems unneccessary.  It looks like there is already handling in hasMemoryWrite and isRemovable for lifetime_end.  I think you just need to adjust that logic and possible pass in the removeLifetimeEnds flag.  
I guess my failure to rename `removeLifetimeEnds` after I changed the logic caused some confusion here. It should really be called `removeLifetimeIntrinsics`.

The idea here is to remove all lifetime intrinsics (start and end) for dead objects, until we hit a lifetime start for a live object. The case for lifetime_start below falls through to lifetime_end. Should have added a comment for that.

That's also why I did not touch `hasMemoryWrite` because handling lifetime_start there seemed wrong to me.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:786
@@ +785,3 @@
+      } else if (isLifetimeStart) {
+        // We found a lifetime start that we can't remove, so lifetime ends
+        // that come before it are interesting again and must be kept
----------------
reames wrote:
> You might consider adding the code to handle a value where isLifetimeStart is the only remaining use of an operand, but feel free to defer that until another patch.  You should also check to see if isTrivialDead already catches that case.  
> 
> Looking at your test case, it looks like something *is* removing the lifetime start.  You might want to look into that to make sure you understand why.
See above, the removal of both kinds of lifetime intrinsics is intended, I just chose a misleading name for the flag that guards it.

isIntructionTriviallyDead only handles lifetime intrinsics on undef values.


http://reviews.llvm.org/D11710