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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs marked 5 inline comments as done.
samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:95
+          continue;
+        MachineBasicBlock *Target2 = TargetTerminator.getOperand(1).getMBB();
+        if (!blockIsBefore(Target2, Target) &&
----------------
SjoerdMeijer wrote:
> 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
Good idea 👍🏻 


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:96
+        MachineBasicBlock *Target2 = TargetTerminator.getOperand(1).getMBB();
+        if (!blockIsBefore(Target2, Target) &&
+            (Target2 == Preheader || blockIsBefore(Target2, Preheader))) {
----------------
SjoerdMeijer wrote:
> 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.
I've added explanations like what you wrote. Let me know what you think.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:105
+      }
+
+      if (CanMove) {
----------------
SjoerdMeijer wrote:
> And after this, we need to check for other instructions in blocks in between?
Indeed. I forgot to add checks for LEs in between the loop exit and preheader, but have added them in the latest updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385