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

Wed Dec 2 03:31:55 PST 2020

samtebbs added a comment.

In D92385#2428006 <https://reviews.llvm.org/D92385#2428006>, @SjoerdMeijer wrote:

> I have some high-level questions:
>
> - Are we fighting another optimisations here, some sort of loop-rotate or is this just MBP reshulffling blocks in a way that is not good for us?

Yeah MBP moves the loop blocks (preheader, body etc.) closer together but unfortunately moves the WLSs branch target above the WLS. So it does some good things but also some bad things. After looking at MBP I thought it would be simpler to make a target pass rather than juggle things around in MBP that could end up affecting other targets.

> - I know we don't nest WLSTPs for profitability reasons, but in theory we could. Not sure we need to check this though. But in general my impression is that some more test can be added, but perhaps you were still working on that.

I wasn't sure if it was possible or not, but thought I'd add the checks in there just in case. I'm OK with removing them if they definitely are unnecessary. I'm also working on testing a nested while loop, can you think of any other tests I should add?

> - I am wondering if some cost-modeling is required. For example, if the iteration count is very low, would that change things?

That is a good idea. It would be good to get an idea of the cycles saved by converting the while loop into a LOL compared to those needed by the branches that replace fallthrough.

================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoopBlockPlacement.cpp:37
+      MachineBasicBlock *Target = MI.getOperand(1).getMBB();
+      if (BB.isLayoutSuccessor(Target))
+        continue;
----------------
samparker wrote:
> 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 :)
I had definitely misinterpreted what being a layout successor means then! Thanks for raising this, I'll have a go at using offsets.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385