[PATCH] D43269: [MemorySSA] Be less aggressive with @llvm.lifetime.start

[Daniel Berlin via llvm-commits]

On Thu, Feb 15, 2018 at 9:20 PM, George Burgess IV via Phabricator <
reviews at reviews.llvm.org> wrote:

> george.burgess.iv added reviewers: reames, hfinkel.
> george.burgess.iv added a subscriber: gberry.
> george.burgess.iv added a comment.
>
> +hal and philip, as requested. +gberry in case they have any earlycse
> thoughts
>
> > So, i don't know what we *want* the semantic to be.
>
> Without much knowledge of the implications elsewhere, something like:
> "lifetime.start intrinsics represent a non-volatile, non-atomic clobber of
> the given range of memory for the purposes of analysis. Anything loaded
> from this range that has not been stored to since the lifetime.start is
> `undef`" doesn't sound insane to me.
>
> (At least, I don't see why we need to give clobber mechanics to both
> lifetime.start and lifetime.end, given that AIUI they're meant to be
> balanced)
>
> > Can you print the memoryssa dumps for this for me?
>
>
>
>   define void @bar(i8* %foo) {
>   entry:
>     %z = bitcast i8* %foo to i32*
>     %p = getelementptr inbounds i32, i32* %z, i64 1
>   ; MemoryUse(liveOnEntry)
>     %a = load i32, i32* %p
>   ; 1 = MemoryDef(liveOnEntry)
>     call void @llvm.lifetime.end.p0i8(i64 8, i8* %foo)
>   ; 2 = MemoryDef(1)
>     call void @llvm.lifetime.start.p0i8(i64 8, i8* %foo)
>   ; 3 = MemoryDef(2)
>     store i32 0, i32* %z
>   ; 4 = MemoryDef(3)
>     store i32 %a, i32* %p
>     ret void
>   }
>
> It's all pretty straightforward IMO; the problematic interaction with
> EarlyCSE is that we don't report that `4` is clobbered by `2`, so AIUI it
> sees `4` as a redundant store.
>

Yeah this isn't a helpful printout because i assume what happens is
getClobberingMemoryAccess(4) returns LiveOnEntry or something.

Here, 4 *isn't* clobbered by 2.

I"m going ot reorg your message

> (I realize I'm saying EarlyCSE a lot in a MSSA patch. The reason I've
> fixed it here is that I believe that, at the very least, MSSA users should
> have to opt into a 'be aggressive with lifetime intrinsics' footgun. Until
> there's complaining, ...)
>

This is almost unrelated (IE it's not "aggressive" handling)
It's really a question about whether MemorySSA is talking about the memory
locations, or the abstract memory objects that llvm has built using the
lifetime system (which only really exist in the lifetime system).

If it's about the former, it's correct as-is.
If it's about the latter, yeah, then we should represent and push it.

The problem is we mix these all over the place, instead of being consistent.

I will say really want to avoid going down the path we did with memdep
here, where it would report fake clobbers and such.

If we want these "memory objects" to be a first class citizen in LLVM, we
should make them such, have the lifetime system return tokens, and use the
tokens in loads/stores so you can tell what memory object they are talking
about.

Right now it just doesn't exist outside of the lifetime system, and we are
just taking the existing mess we have now, and trying to paper over it by
pretending or not pretending, in various places, that lifetime intrinsics
actually affect memory, depending on how we feel at a given time.


>
> > Essentially, lifetime.start/end, even for the same pointer, create
> things that are very two distinctly different memory objects, regardless of
> if they are at the same place in memory, and the compiler is free to treat
> them that way, at least, as these intrinsics go.
>
> Agreed. So, at it's core, the DSE by EarlyCSE is effectively saying "I'm
> eliminating this store to object $X because it's a load of a value from
> object $Y that happens to reside at the same memory location."
>

If memorySSA is about memory locations, it's earlycse being wrong by not
also paying attention to lifetime regions.
In that situation, i don't think we should hack around it, i think we
should fix it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180215/c65d4ad1/attachment.html>