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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:18
 #include "ARMBasicBlockInfo.h"
+#include "ARMLowOverheadLoopBlockPlacement.h"
 #include "ARMMachineFunctionInfo.h"
----------------
left over from prototyping?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.cpp:29
+  bool Changed = false;
+  for (auto It = MF.begin(); It != MF.end(); It++) {
+    MachineBasicBlock &BB = *It;
----------------
I would hope that MachineFunction provides a nicer way to iterate through the blocks? for (auto MBB : MF)?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.cpp:33
+    // the target if so
+    for (auto &MI : BB.instrs()) {
+      if (MI.getOpcode() != ARM::t2WhileLoopStart)
----------------
Visiting these in reverse order will be faster as we're looking at the terminators... so I guess you could just iterate using the terminators() method :)


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.cpp:37
+      MachineBasicBlock *Target = MI.getOperand(1).getMBB();
+      if (BB.isLayoutSuccessor(Target))
+        continue;
----------------
I'm not sure this is what you want to be checking... We need to ensure that Target has a positive branch offset from WhileLoopStart, but that doesn't mean it needs to be the layout successor - I would have even expected that the loop preheader would be the layout successor in many cases? Am I missing something here?



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.h:7
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/Pass.h"
+
----------------
Do you need to include ARMSubtarget.h and Pass.h here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385