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

Thu Feb 15 23:38:15 PST 2018

On Thu, Feb 15, 2018 at 11:08 PM, George Burgess IV <
george.burgess.iv at gmail.com> wrote:

> > Yeah this isn't a helpful printout because i assume what happens is
> getClobberingMemoryAccess(4) returns LiveOnEntry
>
> Yup!
>
> > I will say really want to avoid going down the path we did with memdep
> here, where it would report fake clobbers and such.
>
> Yeah, I don't immediately see a good way to fold lifetimes in MSSA without
> treating turning lifetime.start/lifetime.end into some sort of clobber. If
> that's not a path we want to go down, I'm happy to stick with "MSSA cares
> about memory locations, not abstract objects," and drop this patch. I'll
> give the people I tagged time to respond though. :)
>

Yeah, i'm not doing enough llvm work these days to really be the person who
should say what happens, because i kinda don't have to live with the
consequences :)

>
> ----
>
> Also, sorry if I'm being dense, but if the goal is for MSSA to not care
> about lifetimes, and lifetimes aren't clobbers, why are we representing
> lifetime intrinsics in MSSA at all (alternatively: why does
> instructionClobbersQuery ever return true for lifetime_start)?
>

I don't honestly remember why we did this.
:)

>
> On Thu, Feb 15, 2018 at 9:56 PM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
>
>>
>>
>> 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/d7d6a4b6/attachment.html>