[PATCH] D90940: [LoopFlatten] Make it a FunctionPass

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar.h:334
+//
+FunctionPass *createLoopFlattenPass();
+
----------------
dmgreen wrote:
> Is there an important ordering to this file? It's not alphabetical..
> 
> Any reason why it's moved?
Thanks for looking at this Dave.

I thought of clustering it with the other FunctionPasses, but now I see that's not really true (it's mixed already), so will move it back.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:535
+  bool Changed = false;
+  for (auto *OuterLoop : LI->getTopLevelLoops()) {
+    if (OuterLoop->getSubLoops().size() != 1)
----------------
dmgreen wrote:
> Should this be handling sub-loops too? Maybe use getLoopsInPreorder if that works how I think it does?
Ah, I guess so. This will flatten only doubly nested loops, but won't flatten j and k in a 3d loop:

    for i
      for j
        for k

Looks like a test is missing too...


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:572
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     getLoopAnalysisUsage(AU);
     AU.addRequired<TargetTransformInfoWrapperPass>();
----------------
dmgreen wrote:
> Does this preserve all the things a loop pass would? I think the answer is yes, but it's perhaps worth checking. LI seems to  be kept up to date, which was the one I was worried about.
Will double check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90940/new/

https://reviews.llvm.org/D90940