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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 8 inline comments as done.
chandlerc added a comment.

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



================
Comment at: include/llvm/Analysis/LoopPassManager.h:154-155
+
+  /// \brief Handler for invalidation of the Module.
+  ///
+  /// If the proxy analysis itself is preserved, then we assume that the set of
----------------
davide wrote:
> invalidation of the Module or the Function?
This comment and surrounding comments were all wrong. Updated.


================
Comment at: include/llvm/Analysis/LoopPassManager.h:197-199
+template <typename RangeT>
+inline void appendLoopsInRPO(RangeT &&Loops,
+                             SmallPriorityWorklist<Loop *, 4> &Worklist) {
----------------
davide wrote:
> It could be just me, but I never saw RPO on a tree (only on graphs). Therefore, maybe this function can be called `appendLoopsInPreorder` as it's what the function is doing (and move the comment explaining why the two forms are equivalent on the top of the function).
I'm not sure why "preorder" is more or less confusing for a tree than "reverse postorder"?

Anyways, I agree RPO isn't that useful in the API. I've gone for a more high-level 'appendLoopsToWorklist' and made the adjustment to the comments. Let me know if this works?


https://reviews.llvm.org/D28292