[PATCH] D22966: GVN-hoist: compute MSSA once per function (PR28670)

[Daniel Berlin via llvm-commits]

On Fri, Jul 29, 2016 at 12:14 PM, Sebastian Pop <sebpop at gmail.com> wrote:

> sebpop added inline comments.
>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:761
> @@ +760,3 @@
> +            // otherwise when we try to remove OldMemAcc, MSSA gets
> confused and
> +            // removes NewMemAcc as well.
> +            MSSA->removeMemoryAccess(OldMemAcc);
> ----------------
> dberlin wrote:
> > What do you mean here?
> > That either sounds like a bug in memssa, or something :)
> My first patch had both MemUses and MemDefs handled in the same way.
>

What bug did it cause.
You should be able to delete them in either order, and i don't see why MSSA
would break?

I had a hard time finding how to get around this, so I would rather amend
> the MSSA to avoid other people getting through the same problems.
> Let me think how to fix this in MSSA.


What precisely happens?


>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:770
> @@ +769,3 @@
> +            // NewMemAcc before removing OldMemAcc.
> +            OldMemAcc->replaceAllUsesWith(NewMemAcc);
> +            MSSA->removeMemoryAccess(OldMemAcc);
> ----------------
> dberlin wrote:
> > Are you guaranteed that anything that conflicted with the old store
> conflicts with this store by your hoist safety rules?
> > (if not, you should only replace  inMD's and phis, not memoryuses)
> >
> > it might be worth either giving an example here, or adding a testcase.
> >
> > The two problems are:
> >
> > 1 = MemoryDef<liveOnEntry>
> > store
> > 2 = MemoryDef<1>
> > store
> >
> > later
> > memoryphi(2, )
> >
> >
> > If the hoist point is 1, and you simply move the store, you will now
> have:
> > 1 = MemoryDef<liveOnEntry>
> > store
> > 3 = MemoryDef<1>
> > store
> > 2 = MemoryDef<1>
> > store
> >
> > later
> > memoryphi(2, )
> >
> >
> > This is wrong.  3 is now disconnected from the graph of reaching defs.
> >
> > Additionally, if the hoist point is 2, you end up with:
> > 1 = MemoryDef<liveOnEntry>
> > store
> > 2 = MemoryDef<1>
> > store
> > 3 = MemoryDef<1>
> > store
> >
> > later
> > memoryphi(2, )
> >
> > The memoryphi is now wrong (and you disconnected 2 from the reaching
> graph).
> >
> I added that comment above Def to make sure it is clear that hoisting
> guarantees these properties:
>
>           // The definition of this ld/st will not change: ld/st hoisting
> is
>           // legal when the ld/st is not moved past its current definition.
>           MemoryAccess *Def = OldMemAcc->getDefiningAccess();
>
> > hoist point is 1
> Cannot happen: we cannot move the store "3" past store "2" as they are on
> the same chain (dependent.)
> This is an illegal case of hoisting and will be discarded.
>
> > hoist point is 2
> Cannot happen either as there is a use of "2" (the phi node) past which
> "3" cannot be moved.
> Again the dependence analysis will discard this hoisting as invalid.
>
>
>
> https://reviews.llvm.org/D22966
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160729/a3466968/attachment.html>