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

Tue Dec 15 01:23:35 PST 2020

SjoerdMeijer added inline comments.

================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:77
+      // Make sure that moving this block would not cause a WLS in the target BB
+      // to branch backwards
+      bool CanMove = true;
----------------
samtebbs wrote:
> SjoerdMeijer wrote:
> > Are we here actually not analysing a loop nest? I.e., look for t2WhileLoopStart within a t2WhileLoopStart?
> > 
> > If so, I am not sure that is necessary, because as far as I know that's not something we support for performance reasons? Then, we could perhaps just add an assert on getLoopDepth on the MachineLoop, and just don't need the loop on 79. Maybe I'm overthinking this, but it's just the repetition that makes me wonder. Also, this case doesn't seem to be covered in the tests?
> Indeed it is checking for a nested while loop. If that situation won't occur then I don't mind removing it, or replacing it with an assertion.
> 
> It is tested by the `backwards_branch_nested` function in the mir test file.
Ah yeah, thanks, I missed that test but see it now.
Keeping the test is a good thing, but looks like you need to think how you want to support nesting.
The current implementation is incomplete/not very generic as it only looks 2-deep.
So, all together, best and easiest to get rid of it I think (and probably a return is nicer than an assert).

If I am not mistaken I think some more tests are required with different top level loop nests (1-deep), testing   the different combinations of forward/backward branches to see how reshuffling blocks of 2 loop nests interact.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385