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

[Sjoerd Meijer via Phabricator via llvm-commits]

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;
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:65
+      bool CanMove = true;
+      for (auto &MI : Target->terminators()) {
+        if (MI.getOpcode() != ARM::t2WhileLoopStart)
----------------
samtebbs wrote:
> SjoerdMeijer wrote:
> > I think we are duplicating work here. I.e., we are visiting a block and terminators that we will visit later or have seen already. It shouldn't be a big deal, but the code is also a bit of a copy-paste of the loop starting at line 53.
> > Can we not do one scan of the blocks and record a map of WLSTP to blocks or something like that?
> I did think about that, but didn't think that looping over a list that I've gotten from a map would be more efficient than looping over a list that I've gotten from a MachineBasicBlock (i.e. WLSMap.get(Target) vs Target->getTerminators()). Perhaps I'm not understanding exactly what you want the map to represent.
Okay, yeah, I tried to read this code a bit too quick and was vague. 
Now reading this a bit more carefully, let me ask a more concrete question on line 77.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385