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

[Hal Finkel via llvm-commits]

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

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

> > (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.
> 
> FWIW, i'm fine with this approach if our approach is going to be as
> you say - that we will not simplify often enough.

> Right now, as i said, we simplify *everywhere*, and every one of
> those calls will eliminate this phi node.

> So it's only *this particular path* that misses all those calls.

> For example, if the alias check had gone through a gep of a phi
> anywhere, it would simplify the phi as part of getunderlyingobject,
> etc.

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

> Fine with this as long as we maybe stop trying to simplify
> instructions 8 or 9 times, and instead do it once (max) per
> instructions, and make this part of it.

> (IE This would mean we would have SimplifyAndGetUnderlyingObject and
> GetUnderlyingObject, and we simplify once and call the latter or
> something, or whatever. Not suggesting we decide this second, just
> suggesting that we agree if this is going to be our general
> approach).
This is hard because we don't cache the simplifications in any way. It is not like we're updating the IR when we discover some simplification; we're only using the simplified version in place. I'm not sure how to fix this. Maybe we should run InstSimplify a lot more often. It is not as expensive was InstCombine by a large margin (IIRC). 

> > (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.
> 

> Yes.
> It's theoretically possible to make it less expensive than blowing
> away the whole cache, but so far, experience has told me that fully
> maintaining the cache becomes slower than redoing the queries :P
I'm not sure how much this helps, but we could/should only clear out the MayAlias results. 

-Hal 

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

-- 

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/1ef24b30/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/1ef24b30/attachment.gif>