[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/ARMLowOverheadLoopBlockPlacement.cpp:37
+      MachineBasicBlock *Target = MI.getOperand(1).getMBB();
+      if (BB.isLayoutSuccessor(Target))
+        continue;
----------------
samparker wrote:
> samtebbs wrote:
> > 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.
> Yes, there are linked but is that what you want? isLayoutSuccessor simply compares the iterators for the list of blocks, so in your example B->isLayoutSuccessor(D) == false. In basic block land, successor and predecessor will always means immediate child and parent in the list, whereas it sounds like you're expecting it to work like dominator information but what also wouldn't be suitable for explicitly checking target offsets.
I can't seem to edit my above comment...
> successor and predecessor will always means immediate child and parent in the list
I meant CFG, not list :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385