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

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:737
@@ +736,3 @@
+    // then on, lifetimes are interesting for stack coloring and must be kept
+    bool isLifetimeStart = false;
+    if (removeLifetimeEnds)
----------------
The isLifetimeStart variable appears redundant.  I'd just check if BBI is a lifetime start at the one place you use this.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:739
@@ +738,3 @@
+    if (removeLifetimeEnds)
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI))
+        switch (II->getIntrinsicID()) {
----------------
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.  

================
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
----------------
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.

================
Comment at: test/Transforms/DeadStoreElimination/lifetime.ll:39
@@ -36,1 +38,3 @@
 
+define void @test3() personality i32 (...)* @__gxx_personality_v0 {
+; CHECK-LABEL: test3
----------------
Remove the personality function if it's not required please.

================
Comment at: test/Transforms/DeadStoreElimination/lifetime.ll:42
@@ +41,3 @@
+  %a = alloca i8
+  invoke void @callee(i8* undef)
+    to label %normal unwind label %unwind
----------------
The fact this is an invoke isn't relevant to the test case is it?  If not, please use a call.  If you're just using this to break the block, consider a branch on an i1 argument.

================
Comment at: test/Transforms/DeadStoreElimination/lifetime.ll:57
@@ +56,3 @@
+  call void @llvm.lifetime.start(i64 1, i8* %a)
+; CHECK-NOT: lifetime.start
+  call void @llvm.lifetime.end(i64 1, i8* %a)
----------------
I don't see where this check-not comes from in the code.  Can you explain?  This makes me slightly nervous that I'm not understanding the patch.  (Feel free to revise and then explain/investigate only if this artifact persists.)

================
Comment at: test/Transforms/DeadStoreElimination/lifetime.ll:87
@@ +86,3 @@
+; CHECK: lifetime.start
+  call void @llvm.lifetime.end(i64 1, i8* %a)
+; CHECK: lifetime.end
----------------
Same here.


http://reviews.llvm.org/D11710