[llvm-commits] [patch] Use the new edge dominance functions in GVN

[Daniel Berlin]

On Sat, Aug 11, 2012 at 1:05 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> He's also trying to reconcile the fact that i get about 1 hour to work
>> on it a week at this point, due to dealing with chandler-made-stuff,
>> so he's politely trying to find a way to get it done without
>> mentioning that it's 1+ month late :)
>
> Since pr13307 is part of one of the performance regressions we are
> seeing, I might be able  to put an hour or so in helping with the gvn
> rewrite :-)
>
>>> I will take a look, but I would also like to fix these small issues on
>>> the current one. If nothing else it provides testcases for the new one
>>> :-)
>>
>> Don't take this the wrong way, but the "small issues' have been piling
>> up with the old one for a long time.  It's pretty crufty at this
>> point.  For example, the edge dominance stuff you are doing to solve
>> 13307 would be better taken care of by implementing the predicate
>> support + value inference from the new algorithm, and making it fast
>> (I had implemented it, decided i wouldn't have time to make it fast
>> for the first commit, and ripped it back out).
>
> Do you have a pointer to a description of this algorithm?

Sure, it's "A sparse algorithm for predicated global value numbering"
by Karthik Gargi.
It should be easy to follow the paper compared to the implementation
you'll see on github.


>
>> It takes care of issues like this in a nice, clean way, without having
>> to try to guess locally substituting which equalities will turn out
>> better later on.
>
> Yes that is a problem with the current one. The patch I am working
> right now just exposes more opportunities. To actually fix 13307 I
> need some heuristic on how to use the equality. In that test replacing
> null with %0 is the right thing to do as it simplifies the phi.
>
>> The current algorithm relies *heavily* on catching everything in a
>> single iteration so the next iteration gets the advantage, rather than
>> having a clean separation between analysis and removal, and making the
>> analysis work well, etc.  If you take your example from 13307, and
>> through another variable in it that gets copies of x, etc, you will
>> see it starts to take more iterations to accomplish the same thing.
>>
>> This is actually a fairly bad and slow way to accomplish things in the
>> long run. It works pretty well for small cases, but, for example, as I
>> showed nick, it means that it turns local load dependency checking
>> into slow non-local load dependency checking as it removes loads on
>> each iteration, etc.
>
> I see. So the main point of your rewrite is to replace the current
> iterative replacement with one "complete" analysis step followed by a
> single replacement step, is that it?

Basically, yes. This is a bit tricky for loads, you at least need some
form of iterative load/store numbering, but it doesn't need to use
replacement to do this.

The analysis algorithm is theoretically iterative, but
1. it does not require clearing the value number tables between
iterations, like the current algorithm
2. It does not iterate all instructions on each iteration, it iterates
only the instructions whose value could change as a result of the
changes on the last iteration.

As such,
1. it is guaranteed to take at most N-1 iterations, where N is the
number of iterations the current algorithm performs
2. Each iteration will do less work than the current algorithm.

>
>>
>> Anyhoo, the github branch is https://github.com/dberlin/llvm-gvn-rewrite
>>
>> It's likely slightly broken at this exact moment, i was in the midst
>> of fixing up a small issue, and needed to leave for chicago, so i
>> pushed what i had before i left.
>>
>> I'll push a fixed version when i get back to my laptop on monday.
>>
>> (The version there will be faster in most cases, but slower in some
>> degenerate cases. If you will profile it, you will see all of those
>> cases spend 90+% of their time in getNonLocalPointerDependency, which
>> I was going to fix as a followup).
>
> Thanks. If I am able to put some time on it, what do you think would
> be the best thing for me to work on? Implementing the "predicate
> support + value inference" so that it passes the tests I included in
> my patch?

I would do predicate support, and inferene from predicates. general
value inference is more expensive and harder to find testcases for.

I have support in there now for the propagateEquality style of
predicate inference + replacement (so that we did not lose out
testcases to the current version).    You need to wait for me to push
the latest code to get a few more of the testcases (the version on
github will place the X == true style replacements into the congruence
class values, but not use them during elimination).

>
> In any case,  an updated patch is attached. I made it slightly less
> aggressive but faster by keeping isOnlyReachableViaThisEdge as a fast
> dominance approximation. I understand that it is taking a technical
> debt, but I think I should be able to repay it :-)
>
> Cheers,
> Rafael