[PATCH] D140255: [LoopUnrollAndJam] Visit phi operand dependencies in post-order

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for the patch. It looks OK to me.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:142
 
-  while (!Worklist.empty()) {
-    Instruction *I = Worklist.pop_back_val();
-    if (!Visit(I))
-      return false;
+  auto ProcessInstr = [&](Instruction *I, auto &&ProcessInstr) {
+    if (VisitedInstr.count(I))
----------------
It can be:
```
  std::function<bool (Instruction *I)> ProcessInstr = [&](Instruction *I) {
```
Then is needn't pass ProcessInstr as a parameter.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:177
+                           [&AftBlocks, &InsertLoc, &InsertLocBB](Instruction *I) {
+                             if (AftBlocks.count(I->getParent()) && I->getParent() != InsertLocBB)
+                              I->moveBefore(InsertLoc);
----------------
This needs a bit of formatting.

Alternatively, If I is in AftBlocks I don't think it can be in InsertLocBB too? InsertLocBB should a block before the inner loop.


================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/dependencies_visit_order.ll:3
+; RUN: opt -passes=loop-unroll-and-jam -allow-unroll-and-jam -unroll-and-jam-count=4 < %s -S | FileCheck %s
+; RUN: opt -aa-pipeline=basic-aa -passes='loop-unroll-and-jam' -allow-unroll-and-jam -unroll-and-jam-count=4 < %s -S | FileCheck %s
+
----------------
Does it need both run lines?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140255