[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 added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:18
 #include "ARMBasicBlockInfo.h"
+#include "ARMLowOverheadLoopBlockPlacement.h"
 #include "ARMMachineFunctionInfo.h"
----------------
samparker wrote:
> left over from prototyping?
Yes, thanks. I originally had the constant island pass depend on the new pass.


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


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.cpp:33
+    // the target if so
+    for (auto &MI : BB.instrs()) {
+      if (MI.getOpcode() != ARM::t2WhileLoopStart)
----------------
samparker wrote:
> 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 :)
Ah yes, good idea.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.cpp:37
+      MachineBasicBlock *Target = MI.getOperand(1).getMBB();
+      if (BB.isLayoutSuccessor(Target))
+        continue;
----------------
samparker wrote:
> 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?
> 
Aren't being a layout successor and positive offset linked? Is is possible to have one but not the other? E.g if the layout is A, B, C, D and the WLS is in B then C and D would have positive branch offsets and are also layout successors. I can't think of any situations in which the branch offset is positive but the target basic block comes before the WLS in the layout.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.h:7
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/Pass.h"
+
----------------
samparker wrote:
> Do you need to include ARMSubtarget.h and Pass.h here?
I think Pass.h is left over from the guide I followed and Subtarget.h is probably left over from prototyping as well. Thanks for spotting these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385