[PATCH] D28292: [PM] Rewrite the loop pass manager to use a worklist and augmented run arguments much like the CGSCC pass manager.

[Davide Italiano via Phabricator via llvm-commits]

davide accepted this revision.
davide added a reviewer: davide.
davide added a comment.

In https://reviews.llvm.org/D28292#636375, @chandlerc wrote:

> In https://reviews.llvm.org/D28292#635262, @davide wrote:
>
> > 1. You propose as `FIXME` not using RPO. Do you have data/evidence that a different visitation order actually matters? I'm not entirely convinced about it (but I haven't thought about the problem very carefully so).
>
>
> I do? Are you referring to the order of loops in LoopInfo?
>
> The only thing I found surprising is that the natural order of loops in LoopInfo is from the bottom of the function to the top. I haven't checked if it is exactly in postorder of the CFG but it looks close.
>
> I wrote this patch to make us visit loops "forward" program order (top to bottom), and left a FIXME that we might want to reverse them in LoopInfo itself.
>
> As for which order of visit is better, I don't have data at all. It turns out the current loop pass manager processes loops in reverse. However, very nicely, they wrote a long comment about how there wasn't any particular reason. There is some hand waving in it about deleting uses before processing defs, but that actually seems unlikely to matter to me.
>
> The reason I had wanted to process in forward order are three fold:
>
> First and foremost: I find it much less surprising. I wrote a lot of tests assuming it would be forward and was surprised when it wasn't. This is just that I find forward program order (in the absence of something like the bottom-up walk) less surprising in general.
>
> Second: it seems useful to enable SSA propagation of constants and things from unrolling one loop into its sibling. That typically argues for processing defs before uses.
>
> I think my second argument is a bit more compelling than the idea of deleting uses before processing defs. But not by much. Hence the primary rationale was a principle of least surprise.
>
> > 2. You removed a call to `getContext().yield()`. Do you happen to know why it was introduced in the first place?
>
> I removed it? I thought I just added a commented out bit below a FIXME that hopefully explains what needs to happen here. Happy to dig into this though, and maybe I'm staring at the diff badly...


Both comments make sense. Thanks for explaining. LGTM if/when others are happy.


https://reviews.llvm.org/D28292