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

[Davide Italiano via Phabricator via llvm-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

The logic mostly looks fine (RPO walk etc..). Some minor comments, and I'm a little bit confused by the fact you don't pass `SCEV` to this pass anymore. See comments inline.



================
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) {
----------------
This pattern is actually proliferating. How hard is to make this an helper before this patch goes in? (and replace the other uses)


================
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());
----------------
The old PM actually passes SCEV to `sinkLoopInvariantInstruction`. Here you don't, what are the consequences?


================
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>();
----------------
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?


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:370-371
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     AU.addRequired<BlockFrequencyInfoWrapperPass>();
     getLoopAnalysisUsage(AU);
----------------
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.


https://reviews.llvm.org/D28921