[PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming values are the same.
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 15 12:02:37 PDT 2016
On Fri, Jul 15, 2016 at 11:18 AM, Ehsan A Amiri <amehsan at ca.ibm.com> wrote:
> Another question for me: With the fix (3) are we going to still have cases
> that GVN misses for which we need (2) ?
>
No. You should never need #2 because of *GVN*.
As mentioned, in your testcase, there is another bug here. Remember that
GVN iterates until things stop changing. So *the first gvn pass* should
have never left the IR with that redundant PHI node in the first place.
Essentially, it's only because of some other bug, that you discovered this
one :)
FWIW: I'm fixing the first GVN bug right now, which will, humorously, hide
this bug from regular -O2 because GVN is run twice.
Or do we need (2) because in general this kind of phi node is seen in many
> programs? Or something else that I do not see....
>
That is hal's view.
>
> Thanks
> Ehsan
>
>
> [image: Inactive hide details for Daniel Berlin ---07/14/2016 11:08:16
> PM---On Thu, Jul 14, 2016 at 7:49 PM, Ehsan A Amiri <amehsan at ca.]Daniel
> Berlin ---07/14/2016 11:08:16 PM---On Thu, Jul 14, 2016 at 7:49 PM, Ehsan A
> Amiri <amehsan at ca.ibm.com> wrote: > I can measure compile t
>
> 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 11:08 PM
>
> Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all incoming
> values are the same.
> ------------------------------
>
>
>
>
>
> On Thu, Jul 14, 2016 at 7:49 PM, Ehsan A Amiri <*amehsan at ca.ibm.com*
> <amehsan at ca.ibm.com>> wrote:
>
> I can measure compile time impact of (2) and (3) on power. A couple of
> questions/remarks.
>
> 1- As for (2) I don't think there will be a significant compile time
> cost.
>
>
>
> Compile time does not get slower 5% at a time, it gets slower 0.1% or
> 0.05% at a time.
>
> It's not "someone adds thing that is super slow", it is "people add lots
> of little things to lots of functions called lots of times".
>
> As Hal points out, we never cache that we simplified or didn't, or checked
> the phi or didn't, or *anything*, because BasicAA is stateless.
>
> Because BasicAA is stateless, we will do this again and again and again
> (both this phi checking and simplifyinstruction and ....)
>
> This adds up.
>
> If we do not reduce the phi to its unique incoming value, we will end
> up in aliasPHI and we do this check there. It is just too late for some
> cases like the problem here, and we get conservative result. Do I miss
> something here?
>
>
> The point is that we are going to try to simplify this, and every phi, a
> lot of times, both in AA, and elsewhere.
>
>
> 2- I will use Daniel's patch for (3). Depending on how expensive it
> is, we can look at finer grain cache clean ups, as Hal suggests.
>
> 3- I will modify my patch for (2) and modify stripPointerCasts to
> include "seeing through phi". I wanted to separate that move as a next
> step, but Hal disagreed on the code review.
>
> If you disagree with this way of doing the experiments, please let me
> know.
>
> Thanks
> Ehsan
>
> [image: Inactive hide details for Hal Finkel ---07/14/2016 09:35:07
> PM-------- Original Message ----- > From: "Daniel Berlin" <dberlin@]Hal
> Finkel ---07/14/2016 09:35:07 PM-------- Original Message ----- > From:
> "Daniel Berlin" <*dberlin at dberlin.org* <dberlin at dberlin.org>>
>
> From: Hal Finkel <*hfinkel at anl.gov* <hfinkel at anl.gov>>
> To: Daniel Berlin <*dberlin at dberlin.org* <dberlin at dberlin.org>>
> Cc: llvm-commits <*llvm-commits at lists.llvm.org*
> <llvm-commits at lists.llvm.org>>, <
> *reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org*
> <reviews%2BD22305%2Bpublic%2B92ca108e50bc4651 at reviews.llvm.org>>,
> Ehsan A Amiri/Toronto/IBM at IBMCA
> Date: 07/14/2016 09:35 PM
>
>
> Subject: Re: [PATCH] D22305: [BasicAA] Strip phi nodes, when all
> incoming values are the same.
> ------------------------------
>
>
>
>
> ------------------------------
>
> * From: *"Daniel Berlin" <*dberlin at dberlin.org* <dberlin at dberlin.org>>
> * To: *"Hal Finkel" <*hfinkel at anl.gov* <hfinkel at anl.gov>>
> * Cc: *"llvm-commits" <*llvm-commits at lists.llvm.org*
> <llvm-commits at lists.llvm.org>>,
> *reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org*
> <reviews%2BD22305%2Bpublic%2B92ca108e50bc4651 at reviews.llvm.org>,
> "Ehsan A Amiri" <*amehsan at ca.ibm.com* <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* <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* <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.
>
> [image: Inactive hide details for Daniel Berlin
> ---07/14/2016 04:27:43 PM---Actually, can you please attach a .ll file and
> an opt comma]Daniel 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*
> <dberlin at dberlin.org>>
> To: Ehsan A Amiri/Toronto/IBM at IBMCA
> Cc: Hal Finkel <*hfinkel at anl.gov* <hfinkel at anl.gov>>,
> llvm-commits <*llvm-commits at lists.llvm.org*
> <llvm-commits at lists.llvm.org>>,
> *reviews+D22305+public+92ca108e50bc4651 at reviews.llvm.org*
> <reviews%2BD22305%2Bpublic%2B92ca108e50bc4651 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* <dberlin at dberlin.org>> wrote:
>
>
> On Thu, Jul 14, 2016 at 7:09 AM, Ehsan A Amiri
> <*amehsan at ca.ibm.com* <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
> [attachment "graycol.gif" deleted by Ehsan A Amiri/Toronto/IBM]
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160715/bfe6430e/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/20160715/bfe6430e/attachment.gif>
More information about the llvm-commits
mailing list