[PATCH] D92385: [ARM] Add a pass that re-arranges blocks when there is a backwards WLS branch

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:74
+  // Find loops with a backwards branching WLS
+  for (auto ML : *MLI) {
+    MachineBasicBlock *BB = ML->getLoopPredecessor();
----------------
yeah, what the linter says.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:75
+  for (auto ML : *MLI) {
+    MachineBasicBlock *BB = ML->getLoopPredecessor();
+
----------------
I forgot what the exact difference is between `getLoopPredecessor` and `getLoopPreheader`, but I guess calling `BB` `Preheader` will help a bit here.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:89
+      // can't move
+      bool CanMove = true;
+      SmallVector<MachineLoop *, 2> InnerLoops;
----------------
Sorry if I have missed something, but I am afraid I am still / again confused by this "nested loop analysis" business here.
I think one of my previous remarks still holds? I.e., this lacks some generality because we look only at the next loop down in the loop hierarchy? So, if we support nested hardware-loops, and let's assume a 3-deep loop nest and then this will fail. But again, now I am unsure again if we supported nested loops.
But if I assume we do support nested loops, then I don't see the problem, why can't we actually move some blocks? In other words, I think I am missing a proper problem description, perhaps because I haven't thought long enough about this.  





================
Comment at: llvm/test/CodeGen/Thumb2/block-placement.mir:63
+---
+name:            backwards_branch_nested
+body:             |
----------------
Correct me if I am wrong, but the loops in this test are not nested? Looks like 2 loops at the same loop nest level?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385