[PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.

Thu Jul 14 17:54:23 PDT 2016

----- Original Message -----

> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: "Ehsan A Amiri" <amehsan at ca.ibm.com>
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "llvm-commits"
> <llvm-commits at lists.llvm.org>,
> reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org
> Sent: Thursday, July 14, 2016 5:11:20 PM
> Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all
> incoming values are the same.

> Sigh. Even with all the bugs this discovers, this is unsolvable in
> the general case with the current way GVN is set up.

> First, this bug definitely gets fixed by making sure the first GVN
> simplifies the phi so the second GVN run doesn't see it.

> There are bugs in GVN that cause this to not happen right now, and
> fixing them makes it work.
> The game turns out to be over as soon as you have this phi node as
> input to GVN. However, it's sadly worse than that.

> The iteration orders of memdep and GVN are basically the exact
> opposite, so GVN will always have memdep cache things that it will
> later simplify, and as we discussed on the bug, we can't ever get
> them out of the memdep cache.

> This is about more than PRE or phis, it literally affects every
> memdep query it makes where the path involves something GVN will
> later simplify, at all, and that simplification makes it able to see
> a no-alias (or in some cases, a partial/must-alias).

> For example, if you add a pointer that alias queries follow along a
> backedge that requires constant folding, or *anything* basicaa does
> not itself process but gvn/simplifyinstruction does, it will fail in
> precisely the same way.

> Worse than that, without the other patches (from above) to GVN to
> make it stop generating trivially bad IR, *GVN* itself will generate
> these kinds of cases.

> So even with your alias patch, i can trivially modify the testcase in
> a number of ways to make it break again.

> This means while your alias patch will fix the current case, there is
> literally no way to fix all the cases (again, in current GVN. New
> GVN + MemSSA will not have these issues) you are going to find that
> doesn't involve something like blowing away memdep cache entirely on
> each iteration of GVN.

> I'm not sure how we want to paper over this bug:

> 1. Making first run of GVN not generate IR the second GVN will have
> to trivially simplify (which will fix some cases but not others)

> 2. Make BasicAA see through phi nodes (ditto, though chandler points
> out there are plenty of normal programs with thousands of phi nodes
> that have the same arguments, and since basicaa does not cache,
> there is a compile time cost to doing this )

> 3. Add an API and use it to blow away memdep cache entirely between
> GVN iterations (which will fix all such cases at some possible
> compile time cost)

> Some combination of the above?

I won't have a real opinion until we measure the compile-time cost of (2) and (3), but as a preliminary matter, I vote for both (2) and (3). Why? 

(2) is, in theory, the right thing to do. Even if we were to consider uniform PHIs to be anti-canonical, and thus something which should be simplified, we can't simplify often enough to prevent these from blocking useful analysis work. Arbitrary uses of RAUW can create these PHIs, and we can't (and probably shouldn't) run InstCombine in between every other pass. This is a local pattern that stripPointerCasts, and similar functions, can look through. 

(3) is also, in theory, the right thing to do. The memdep cache, by necessity, caches negative results. Each GVN iteration, however, performs "information revealing" operations which can make the cached results more conservative than a new query's results. 

Now we actually need to measure the costs. 

-Hal 

> (Note: i've attached a patch for 3 in case anyone wants to see the
> compile time cost)

> On Thu, Jul 14, 2016 at 2:12 PM, Daniel Berlin < dberlin at dberlin.org
> > wrote:

> > Note: This already had GVN run once on it, do you have the one
> > before
> > that?
> 

> > On Thu, Jul 14, 2016 at 1:53 PM, Ehsan A Amiri < amehsan at ca.ibm.com
> > >
> > wrote:
> 

> > > I can see the problem with the following command line and
> > > attached
> > > file
> > 
> 

> > > opt -gvn t.ll
> > 
> 

> > > (See attached file: t.ll)
> > 
> 

> > > My clang is almost a week old.
> > 
> 

> > > Inactive hide details for Daniel Berlin ---07/14/2016 04:27:43
> > > PM---Actually, can you please attach a .ll file and an opt
> > > commaDaniel Berlin ---07/14/2016 04:27:43 PM---Actually, can you
> > > please attach a .ll file and an opt command line that reproduces
> > > the
> > > problem?
> > 
> 

> > > From: Daniel Berlin < dberlin at dberlin.org >
> > 
> 
> > > To: Ehsan A Amiri/Toronto/IBM at IBMCA
> > 
> 
> > > Cc: Hal Finkel < hfinkel at anl.gov >, llvm-commits <
> > > llvm-commits at lists.llvm.org >,
> > > reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org
> > 
> 
> > > Date: 07/14/2016 04:27 PM
> > 
> 
> > > Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all
> > > incoming values are the same.
> > 
> 

> > > Actually, can you please attach a .ll file and an opt command
> > > line
> > > that reproduces the problem?
> > 
> 
> > > The clang command line you have is very sensitive to versions,
> > > etc.
> > 
> 

> > > I cannot get your issue to reproduce with the clang i have
> > > installed
> > > that can target powerpc-linux (and the issue does not reproduce
> > > with
> > > your testcase on x86) :)
> > 
> 

> > > While debugging a bit, note that there is at least one obvious
> > > bug
> > > in
> > > GVN that may affect this, by inspection:
> > 
> 

> > > When GVN splits a critical edge, it never adds the new block to
> > > the
> > > iteration order (at all), even though it inserts into it.
> > 
> 
> > > So they will not get processed until the next iteration of GVN on
> > > the
> > > function, even though they have code in them. While this is okay
> > > from a correctness standpoint, it may block optimization of
> > > certain
> > > things (including the cases you've discovered). In practice,
> > > there
> > > is no way to perfectly solve that without pre-splitting all
> > > critical
> > > edges, but you should get the same effect if we throw the
> > > critical
> > > edge block and then it's successors (including the current blcok)
> > > into bbvect after the current block again.
> > 
> 

> > > It is also missing a real phi simplification.
> > 
> 
> > > While simplifyinstruction will check if all arguments are
> > > trivially
> > > the same, that is not the real test that should be performed.
> > 
> 

> > > It should be doing VN.lookup on each argument and seeing if they
> > > come
> > > up with the the same value number.
> > 
> 

> > > Once you attach the .ll file, i'll fix these and see if it fixes
> > > your
> > > testcase, and if not, debug further.
> > 
> 

> > > On Thu, Jul 14, 2016 at 10:11 AM, Daniel Berlin <
> > > dberlin at dberlin.org
> > > > wrote:
> > 
> 

> > > On Thu, Jul 14, 2016 at 7:09 AM, Ehsan A Amiri <
> > > amehsan at ca.ibm.com
> > > >
> > > wrote: This is order of events:
> > 
> 
> > > This order cannot be correct if the solution I gave (or adding
> > > simplification to *some place in GVN*) does not work. Or GVN is
> > > broken in other ways. 1) GVN starts looking at the function. At
> > > this
> > > point the phi node has two different incoming values. 2) GVN
> > > performs an RAUW. The phi is converted to the one that has two
> > > identincal incoming values.
> > 
> 
> > > At this point, it should now process the phi instruction again
> > > before
> > > it processes the load, because it is doing a reverse postorder
> > > traversal. When it did that, the phi should have been simplified
> > > So
> > > why did that not happen? Given the complexity of fixing the real
> > > problem,
> > 
> 
> > > Look, i understand why you want to just fix this in AA and be
> > > done
> > > with it. Really, I do. I understand you have spent a lot of time
> > > on
> > > this bug, and I greatly appreciate that. But I really want to
> > > understand what is going on before we try to actually fix it. I
> > > have
> > > a good understanding of what happens once the bad answer gets
> > > into
> > > memdep (and thank you for that!), but i still have trouble seeing
> > > why it lived long enough to get there. To that end, so you don't
> > > have to spend more time running around for me, i'll take over
> > > this
> > > bug, and either figure out why GVN lets this PHI live to the
> > > point
> > > it gets an AA query about it (and fix it/decide it can't be
> > > fixed),
> > > or commit the AA patch for you if we decide it can't be fixed. My
> > > ETA is by friday. I assume the testcase in the bug is the one we
> > > are
> > > still using, right? (If not, if you can attach it, that would be
> > > helpful)
> > 
> 

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160714/f7402eee/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160714/f7402eee/attachment.gif>