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

[Daniel Berlin via llvm-commits]

On Tue, Sep 13, 2016 at 1:55 PM, Sebastian Pop <sebpop at gmail.com> wrote:

> > 1. No MemorySSA form handles *hoisting* stores (not gcc, not open64's
> > mu/chi/etc).
> > It requires building the reverse problem to make these links exist.
>
> Would it be possible to provide an inverse MemorySSA graph:
> like a CFG df_walk has the idf_walk, except as you are saying,
> that for MemSSA we would have to build the inverse graph that
> is not just an upside-down version of MemSSA.
>

Yes.

Consider:
load a
if ()
  store a
else
  store a

SSU (as defined by the SSAPRE paper. There are at least 3 things called SSU
in literature), would create:

load = use of mem1
mem2, mem3 = lambda (mem1)
if ()
  store = def of mem2
else
  store = def of mem3


Now you can at least recursively walk the uses of the stores and lambdas.

Right now you would have to walk the uses of the loads.


2. you just generate a form for WAR dependence.

This is "immediate upward use".


> MemSSA represents RAW and WAW dependences.
> The inverseMemSSA would represent WAR dependences.
>


>
> > Instead, most things *sink* stores, which MemorySSA makes easy.
> > See SSUPRE, etc.
> >
> > 2. If you are going to write your own walkers, etc, we don't make it easy
> > enough.
> >
> > My initial view is: Rather than do this as a completely separate piece by
> > piece query interface, the intention was for folks to write custom
> walkers,
> > and use those.
> > For example, the code in this patch could be written as a MemorySSA
> walker
> > that implemented "getClobberingMemoryAccess" for stores.
> > It has a different definition of what it means to be a clobbering memory
> > access than the default walker.
> > That's okay.
> >
> > *Inside of walkers*, we should make instructionClobbersQuery available.
> >
> > Thoughts?
>
> I'm not sure the MemSSA will have all the links to be able to walk
> the WAR dependences.
>

It does not.

It's also not clear to me it should. AFAIK, most things simply common
stores  by sinking rather than hoisting.

Obviously, you can do both because they don't cover each other (you can
sink stores you can't hoist, you can hoist stores you can't sink), and you
would need to do both to be optimal, code size wise.



>
> In the mean time, is the current patch suitable to commit to fix the
> miscompile?
>
>
I'll review in a few moments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160913/b6d495fe/attachment.html>