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

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/minor comments addressed.  No further rounds of review needed.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:852
@@ -851,1 +851,3 @@
 
+  bool removeLifetimeIntrinsics = true;
+
----------------
Naming wise, please use canRemoveLifetimeIntrinsics.  This is a legality check, so let's make that obvious.  

Add a one line comment to describe purpose. e.g.
// becomes false once lifetime intrinsics are observable or useful for stack colouring

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:864
@@ +863,3 @@
+          case Intrinsic::lifetime_start:
+          case Intrinsic::lifetime_end:
+            V = II->getArgOperand(1);
----------------
I still think that the handling you've added could be done by extending getStoredPointerOperand, hasMemoryWrite, and isRemovable, however, I won't require this.  These are used in enough other places that changing them would require a good amount of care and is somewhat risky for minimal gain.  I'd still prefer that - it seems like the better long term choice - but I won't hold the review on that.  

What bugs me about the special casing is that lifetime begin and end *are* stores.  They just store undef into the memory instead of a real value.  (Caveat. I believe that's a conservatively correct interpretation, but I think the actual semantics are a bit less strict per previous mailing list discussions.)

================
Comment at: test/Transforms/DeadStoreElimination/lifetime.ll:14
@@ -12,3 +13,3 @@
 
   store i8 0, i8* %A  ;; Written to by memset
   call void @llvm.lifetime.end(i64 1, i8* %A)
----------------
It looks like this store is also now dead, mind adding an check line for that?


http://reviews.llvm.org/D11710