[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:73
+
+  // Find loops with a backwards branching WLS
+  for (auto *ML : *MLI) {
----------------
Can you describe the algorithm here in comments?


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:80
+        continue;
+      MachineBasicBlock *Target = Terminator.getOperand(1).getMBB();
+      if (blockIsBefore(Preheader, Target))
----------------
Nit: think my preference would be to call this "LoopExit", just to to avoid any potential confusion that this jumps to the loop header.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:95
+          continue;
+        MachineBasicBlock *Target2 = TargetTerminator.getOperand(1).getMBB();
+        if (!blockIsBefore(Target2, Target) &&
----------------
Nit: would be best if we could rename Target2 to something more descriptive.
But perhaps LoopExit2 is good enough, but then I think an example would help me here:

  bb1:       // LoopExit
    WLS xyz  // LoopExit2
  bb2:       // Preheader
    WLS bb1
  bb3:        // Header


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:96
+        MachineBasicBlock *Target2 = TargetTerminator.getOperand(1).getMBB();
+        if (!blockIsBefore(Target2, Target) &&
+            (Target2 == Preheader || blockIsBefore(Target2, Preheader))) {
----------------
To understand all cases for LoopExit2, can we first list them here:

  
   -  the WLS can be forward branch:
    -- if it branches to bb2, then we would create a backward branch. So, we would fix one, and regress one, and don't really win anything, unless the one that we fix has is "more important to fix".
     -- if it branches to some block after bb3, then moving bb1 to after bb2, we create 2 forward branches, and all is okay.
   - the WLS can be a backward branch:
    -- if we move bb1, its backward branch remains backward, so that doesn't regress and we fix another and is thus a win.

So, the one case that we recognise here is the first one. We could support this, i.e. perhaps could make a decision if this is a good thing to do, but don't at the moment. So we should put a TODO here explaining that.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:105
+      }
+
+      if (CanMove) {
----------------
And after this, we need to check for other instructions in blocks in between?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385