[PATCH] D28921: [PM] Port LoopSink to the new pass manager.

[Chandler Carruth via Phabricator via llvm-commits]

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

Most comments addressed. Factoring out the loop walk to a helper.



================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:311-326
+
+  // We want to do a postorder walk over the loops. Since loops are a tree this
+  // is equivalent to a reversed preorder walk and preorder is easy to compute
+  // without recursion.
+  // FIXME: We do this in several places, we should factor it into a helper.
+  SmallVector<Loop *, 4> PreOrderLoops, PreOrderWorklist;
+  for (Loop *RootL : LI) {
----------------
davide wrote:
> This pattern is actually proliferating. How hard is to make this an helper before this patch goes in? (and replace the other uses)
Sure. And as Sanjoy spotted, it even has a tiny bug.


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:331-332
+    Loop &L = *PreOrderLoops.pop_back_val();
+    Changed |= sinkLoopInvariantInstructions(L, AA, LI, DT, BFI,
+                                             /*ScalarEvolution*/ nullptr);
+  } while (!PreOrderLoops.empty());
----------------
davide wrote:
> The old PM actually passes SCEV to `sinkLoopInvariantInstruction`. Here you don't, what are the consequences?
It is only used to invalidate the loop in SCEV. Now we just don't preserve SCEV (and don't require it either). I'll add a comment.


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:340-345
+  // TODO: What we really want to do here is preserve an AA category, but that
+  // concept doesn't exist yet.
+  PA.preserve<AAManager>();
+  PA.preserve<BasicAA>();
+  PA.preserve<GlobalsAA>();
+  PA.preserve<SCEVAA>();
----------------
davide wrote:
> As far as I can see the old pass doesn't preserve AA passes.
> I'm fine with this going in but maybe can be separated?
In the new PM, we can actually preserve SCEVAA and not SCEV and it can work! But I don't think its in good shape yet.

And Davide, as you may have noticed below, the getLoopAnalysisUsage makes the old PM preserve AA as well! Yay!

Anyways, I'll just rip out AA preservation for now. We can add it back with intent and clarity later.


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:370-371
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     AU.addRequired<BlockFrequencyInfoWrapperPass>();
     getLoopAnalysisUsage(AU);
----------------
davide wrote:
> Ugh, this seems wrong (not your fault of course). The transform seems to ask for AA and LoopInfo but doesn't quite list them as required.
It does inside of getLoopAnalysisUsage. Yay subtle. =[


https://reviews.llvm.org/D28921