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

George Burgess IV via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 15 23:08:13 PST 2018


> 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. :)

----

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)?

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/84f1348e/attachment.html>


More information about the llvm-commits mailing list