llvm dev - Dec 2012 - [LLVMdev] Can simplifycfg kill llvm.lifetime intrinsics?

On 24 December 2012 04:02, Alexey Samsonov <samsonov at google.com>
wrote:
> This looks like a bug in simplifycfg. We should preserve lifetime
intrinsics
> due to the reasons I described.
> The code in //lib/Transforms/Utils/Local.cpp:
>
>   if (Succ->getSinglePredecessor()) {
>     // BB is the only predecessor of Succ, so Succ will end up with exactly
>     // the same predecessors BB had.
>
>     // Copy over any phi, debug or lifetime instruction.
>     BB->getTerminator()->eraseFromParent();
>     Succ->getInstList().splice(Succ->getFirstNonPHI(),
BB->getInstList());
>   }
>
> does this only when successor of basic block being removed has a single
> predecessor.
> This is not the case even for simple test in
> /test/Transforms/SimplifyCFG/lifetime.ll.
>
> That said, I think for now we should just apply the patch attached to this
> mail. Please take a look.
Sorry I missed this email the first time.
>From http://llvm.org/docs/LangRef.html it looks to be valid to delete
only one of the functions. The semantics being

* only llvm.lifetime.start: Any loads before the call can be replaced
with undef. Since there is no end, all stores must be kept.
* only llvm.lifetime.end: Any stores after this can be deleted. Since
there is no start, all loads must be kept.

I guess with asan the "replace with undef/ remove store" become
"diagnose those loads/stores".

I am pretty sure we should not avoid optimizing because there is a
llvm.lifetime.* in a BB. If we decide that it is invalid to delete one
but not the other, we should change SimplifyCFG to delete both, not
avoid merging the BBs.

Nick, is the above what you had in mind when you added
llvm.lifetime.*? We should clarify the language ref even if it is
valid to drop only one of the calls.

Cheers,
Rafael

On Tue, Dec 25, 2012 at 11:09 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:
> On 24 December 2012 04:02, Alexey Samsonov <samsonov at google.com>
wrote:
> > This looks like a bug in simplifycfg. We should preserve lifetime
> intrinsics
> > due to the reasons I described.
> > The code in //lib/Transforms/Utils/Local.cpp:
> >
> >   if (Succ->getSinglePredecessor()) {
> >     // BB is the only predecessor of Succ, so Succ will end up with
> exactly
> >     // the same predecessors BB had.
> >
> >     // Copy over any phi, debug or lifetime instruction.
> >     BB->getTerminator()->eraseFromParent();
> >     Succ->getInstList().splice(Succ->getFirstNonPHI(),
> BB->getInstList());
> >   }
> >
> > does this only when successor of basic block being removed has a
single
> > predecessor.
> > This is not the case even for simple test in
> > /test/Transforms/SimplifyCFG/lifetime.ll.
> >
> > That said, I think for now we should just apply the patch attached to
> this
> > mail. Please take a look.
>
> Sorry I missed this email the first time.
>
> From http://llvm.org/docs/LangRef.html it looks to be valid to delete
> only one of the functions. The semantics being
>
> * only llvm.lifetime.start: Any loads before the call can be replaced
> with undef. Since there is no end, all stores must be kept.
> * only llvm.lifetime.end: Any stores after this can be deleted. Since
> there is no start, all loads must be kept.
>
Ok, suppose you have the following code:
BB1:
  llvm.lifetime.start(%a)
  store to %a
  llvm.lifetime.end(%a)
  br %BB2

BB2:
  <some code>
  br %BB1

If you remove the first "llvm.lifetime.start", then when you enter
%BB1 for the second time, your "store to %a" can be considered
invalid,
as you've already called llvm.lifetime.end for this variable.

>
> I guess with asan the "replace with undef/ remove store" become
> "diagnose those loads/stores".
>
> I am pretty sure we should not avoid optimizing because there is a
> llvm.lifetime.* in a BB. If we decide that it is invalid to delete one
> but not the other, we should change SimplifyCFG to delete both, not
> avoid merging the BBs.
>
> Nick, is the above what you had in mind when you added
> llvm.lifetime.*? We should clarify the language ref even if it is
> valid to drop only one of the calls.
>
> Cheers,
> Rafael
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121225/84ab9435/attachment.html>

> Ok, suppose you have the following code:
> BB1:
>   llvm.lifetime.start(%a)
>   store to %a
>   llvm.lifetime.end(%a)
>   br %BB2
>
> BB2:
>   <some code>
>   br %BB1
>
> If you remove the first "llvm.lifetime.start", then when you
enter
> %BB1 for the second time, your "store to %a" can be considered
invalid,
> as you've already called llvm.lifetime.end for this variable.
Oh, I was reading "precedes/following" as having static (dominance)
meaning. That is, in the above example you could not delete the store
since it is not true that
llvm.lifetime.end dominates it.

Nick, is this what you had in mind? If not, then we must delete a
matching llvm.lifetime.end, but it is not clear how we define
"matching". In the following code some executions will hit the first
llvm.lifetime.end and others will hit the second one.


BB0:
...
llvm.lifetime.start(%a)
...
br i1 %foo, label %BB1, label %BB2

BB1:
...
llvm.lifetime.end(%a)
...
br label %bar

BB2:
...
llvm.lifetime.end(%a)
...
br label %zed


Cheers,
Rafael

On 12/25/2012 1:31 PM, Alexey Samsonov wrote:
>
> Ok, suppose you have the following code:
> BB1:
>    llvm.lifetime.start(%a)
>    store to %a
>    llvm.lifetime.end(%a)
>    br %BB2
>
> BB2:
>    <some code>
>    br %BB1
>
> If you remove the first "llvm.lifetime.start", then when you
enter
> %BB1 for the second time, your "store to %a" can be considered
invalid,
> as you've already called llvm.lifetime.end for this variable.
The llvm.lifetime.end(%a) in your example is invalid, according to the 
lang ref:

"This intrinsic indicates that after this point in the code, the value 
of the memory pointed to by ptr is dead. This means that it is known to 
never be used and has an undefined value. Any stores into the memory 
object following this intrinsic may be removed as dead."

If you can re-enter the block with a still-defined value of %a, then 
it's not "known to never be used".

-Krzysztof

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation