[PATCH] D13305: [InstCombine] Remove trivially empty lifetime start/end ranges.

[Arnaud A. de Grandmaison via llvm-commits]

aadg added a comment.

I intended to catch another trivially empty ranges in a follow-up patch: the non strictly nested ones you refer to. The goal was to quickly fix an issue (PR24598), then improve the lifetime markers cleanup.

I think we should teach this cleanup to address less trivial cases, especially the one where the start dominates the end (garanteed by design) and the end post-dominate the start and the alloca is not used in between. I think this would catch lots of cases. But that's for another patch.

And the loop unroller probably needs to learn how not to mess to badly with the lifetime markers :).

Thanks for the reviews !

I'll fix and push this patch.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1398
@@ +1397,3 @@
+    // immediately followed by an end (ignoring debuginfo in between).
+    BasicBlock::iterator BI = II, BE = II->getParent()->end();
+    for (++BI; BI != BE; ++BI) {
----------------
chandlerc wrote:
> These should be loop variables.
I agree on the principle, but BI needs to be incremented before entering the loop.

================
Comment at: test/Transforms/InstCombine/lifetime.ll:2
@@ +1,3 @@
+; RUN: opt < %s -instcombine -S | FileCheck %s
+; RUN: opt < %s -instcombine -S | grep lifetime | count 6
+
----------------
chandlerc wrote:
> manmanren wrote:
> > It is probably better to just check the 6 occurrences with a single RUN command.
> This already is checked by the FileCheck line. I think this can just be deleted.
Those 2 RUN lines were a very clumsy way of making sure the "if" basic block has been emptied, and the "else" was not modified. One RUN line is enough indeed, and I will add 2 checks:
    CHECK: if:
    CHECK-NEXT: br label %fin


http://reviews.llvm.org/D13305