[PATCH] D28129: NewGVN: Sort Dominator Tree in RPO order, and use that for generating order.

Daniel Berlin via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 28 13:03:54 PST 2016


On Wed, Dec 28, 2016 at 7:04 AM, Davide Italiano via Phabricator <
reviews at reviews.llvm.org> wrote:

> davide accepted this revision.
> davide added a comment.
> This revision is now accepted and ready to land.
>
> Sorry for the slow response, I'm out('ish) of the office these days. I
> took a close look at your patch.
>

No worries.


> I happen to be lucky enough to hit a case in the wild where this already
> matters.  The number of iteration goes down from hundreds to ~10, which
> makes compile time/me happier.
>

yay.

The current code, excepting super-weird cases, should operate in O(d+3)
iterations, where d is the loop connectedness of the SSA graph (not the
CFG), which is the number of backedges in any path. This will change when
we move to equality propagation, but for now, ...
We could  calculate this number and see if we are screwing up :)

For most programs, the loop connectedness of the SSA graph is the same or
less than the CFG.

However,  IIRC, we allow  dependent phis in the same block (this is not
strictly SSA, since all phi nodes are supposed to be evaluated
simultaneously).

If that is correct, you can construct programs where the loop connectedness
of the SSA graph is much worse than the CFG.


IE
f = phi(h, i)
d = phi(f, g)
b = phi(d, e)
a = phi(b, c)

in the same block.


We can resolve this issue a number of ways, like ordering the phi nodes in
the block topologically, finding and iterating the SCC's specially, blah
blah blah.

There are also other tricks we can play to speed convergence - much like
SCCP, it's an optimistic algorithm, so we start by assuming everything is
the same.  This means the first iteration should be maximal (though maybe
not correct).
Anything that we don't find a congruence for (IE end up in their own class)
on the first iteration, we will never find a congruence for, and there is
no point in revisiting them, no matter what else changes, because our
answer can't get better, only worse.
At least, i believe this is correct, i'll prove it one way or the other.

Note that this is one difference between the branch predicate handling and
the paper predicate handling - the branch predicate handling can cause
significantly more iterations. The paper predicate handling not so much.


Your reasoning looks good to me, so feel free to check this in (modulo
> small nits). I'll do some more extensive testing when I'm in back in
> California early next year (although as you pointed out this is not exposed
> without predicates handling.
>
>
>
> ================
> Comment at: lib/Transforms/Scalar/NewGVN.cpp:1375-1377
> +  // The dominator tree does guarantee that, for a given dom tree node,
> it's
> +  // parent must occur before it in the RPO ordering. Thus, we only need
> to sort
> +  // the siblings.
> ----------------
> I apologize in advance if I'm wrong, as I'm not a native speaker, but it
> shouldn't be
> `its parent` (at least this is the way I've always seen it written down)?
> Please ignore otherwise.
>

Yes it should be its.


>
>
> ================
> Comment at: lib/Transforms/Scalar/NewGVN.cpp:1457
>    }
> -
>    Changed |= eliminateInstructions(F);
> ----------------
> Spurious removal of a newline?
>
>
> Fixed!


> https://reviews.llvm.org/D28129
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161228/f067ffbb/attachment.html>


More information about the llvm-commits mailing list