[PATCH] D99774: [LoopUtils] Populate sibling loops in reverse program order on new pass manager

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:467
 /// Calls the templated utility taking a Range of loops, handing it the Loops
 /// in LoopInfo, iterated in reverse. This is because the loops are stored in
+/// RPO w.r.t. the control flow graph in LoopInfo.
----------------
this is now wrong?


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:468-471
-/// RPO w.r.t. the control flow graph in LoopInfo. For the purpose of unrolling,
-/// loop deletion, and LICM, we largely want to work forward across the CFG so
-/// that we visit defs before uses and can propagate simplifications from one
-/// loop nest into the next. Calls appendReversedLoopsToWorklist with the
----------------
This comment is what worries me the most. The forward order was deliberately chosen with motivation.

Sorry for all the scrutiny, but this is an important change. If everybody else is ok with this change, then I'm fine too. But I would like to know what the source of your test case is. Is it from some C code?

I still think it would be nice if we could somehow, after a loop has been deleted, revisit other loops that contained defs for uses in the deleted loop. That way we still get advantages of forward traversal along with some benefit of reverse traversal. But perhaps that's not reasonable for the loop pass manager to do.


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

https://reviews.llvm.org/D99774