[PATCH] D84413: [DA][SDA] SyncDependenceAnalysis re-write

[Simon Moll via Phabricator via llvm-commits]

simoll added inline comments.


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:267
+    DivLoop = DivLoop->getParentLoop();
+    if (!DivLoop || DivLoop->contains(&DivExit))
+      break;
----------------
sameerds wrote:
> Loop::Contains is called with the same argument in each iteration. If LoopInfo is available at this point, the function could call LoopInfo::getLoopFor() just once outside the current while-loop and then compare pointers at this point.
We cannot do that because`getLoopFor` would give us the inner-most loop that contains `DivExit`. We are looking for the outer-most loop that does not contains `DivExit` here.  I've changed this to use the loop depth instead.


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:141
+      SmallVector<BasicBlock *, 3> NestedExits;
+      NestedLoop->getUniqueExitBlocks(NestedExits);
+      bool PushedNodes = false;
----------------
nhaehnle wrote:
> This is relatively expensive, isn't it? And it gets executed once per loop exit instead of once per loop...
Actually, this is only called once per loop header.


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:224-227
+  ComputeTopLevelPO(*DT.getRoot()->getParent(), LI, [&](const BasicBlock &BB) {
+    // errs() << BB.getName() << "\n";
+    LoopRPO.appendBlock(BB);
+  });
----------------
nhaehnle wrote:
> This looks like it's computing a post-order, not a reverse post-order. I haven't looked into how this lines up with the rest of the algorithm, but perhaps you can rename LoopRPO to LoopPO?
Yes, we compute PO and traverse the block list backwards to do the reversal when it's used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84413