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

[Joshua Cao via Phabricator via llvm-commits]

caojoshua marked 3 inline comments as done.
caojoshua added inline comments.


================
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))
----------------
dmgreen wrote:
> It can be:
> ```
>   std::function<bool (Instruction *I)> ProcessInstr = [&](Instruction *I) {
> ```
> Then is needn't pass ProcessInstr as a parameter.
Yep. Thanks for C++help.


================
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);
----------------
dmgreen wrote:
> 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.
Formatted with clang-format. Removed check for InsertLocBB.


================
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
+
----------------
dmgreen wrote:
> Does it need both run lines?
Nope. Removed second line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140255