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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 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.
----------------
aeubanks wrote:
> this is now wrong?
No, it is not wrong. It calls different function in this change now. The comment was for forward program order.


================
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
----------------
aeubanks wrote:
> 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.
> This comment is what worries me the most. The forward order was deliberately chosen with motivation.

I agree with the comment but there could be other cases get worse with forward order.

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

Yep, the test case comes from spec benchmark and I have reduced it.
 
> 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.

I agree with it. Each pass could re-visit the target loops.


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

https://reviews.llvm.org/D99774