[PATCH] D24517: GVN-hoist: fix store past load dependence analysis (PR30216)

[Daniel Berlin via llvm-commits]

Yes, this is the issue where I said "it assumes certain things about
aliasing" :)


On Wed, Sep 14, 2016 at 9:04 AM, Geoff Berry <gberry at codeaurora.org> wrote:

> I don't think this is right.  Consider attempting to hoist the store %p
> just before the load in the following case:
>
> store @A ; 1 = MemoryDef(LiveIn)
> load @B ; MemoryUse(LiveIn)
> store %p ; 2 = MemoryDef(1)
>
> You won't see the load as a use of store @A because it has been optimized
> to point to LiveOnEntry, so it will appear that it is safe to hoist it when
> it is not.
>
>
> On 9/13/2016 7:38 PM, Daniel Berlin wrote:
>
>
>>> I'm not sure the MemSSA will have all the links to be able to walk
>>> the WAR dependences.
>>>
>>
>> It does not.
>>
>
> Actually, you can build a walker already that will give you war dependence
> info.
>
> Here is the pseudo code for a getClobberingAccess (note that it is faster
> to build the entire aliased load set than to call this multiple times.
> Noted below what to change to do that).  Note that it assumes transitivity
> in aliasing, but this is easy to fix by changing the outerworklist to also
> include store uses.
>
> Initialize OuterWorklist to be cachingWalker->getClobberingAccess(
> OriginalStore)
> while (!OuterWorklist.empty())
>   CurrentStore = OuterWorklist.pop_front();
>
>   // Any load that is aliased with the store must be a user of the store
> in use-optimized memoryssa
>   for each User of CurrentStore:
>     if (isa<MemoryUse>(User))
>       UseWorklist.push_back(User)
>
>   while (!UseWorklist.empty())
>     CurrentAccess = UseWorklist.pop_front();
>     if (StoreClobbersLoad(OriginalStore, CurrentAccess)) // This is
> instructionClobbersQuery, basically
>         return CurrentAccess // If you change this to insert it into a set
> instead of returning, you can get the entire set of dominating aliased loads
>
>   // Keep going up until we've gotten past the attempted hoist point
>   if (!MSSA->dominates(CurrentStore, HoistPoint))
>     OuterWorklist.push_back(cachingWalker->getClobberingAccess(
> CurrentStore->getDefiningAccess()))
>
>
> Note that this should check <= the number of loads than a version that
> walks all loads does.
>
>
> --
> Geoff Berry
> Employee of Qualcomm Datacenter Technologies, Inc.
>  Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160914/8cc5bdac/attachment.html>