[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/ARM.h:63
 
 void initializeARMParallelDSPPass(PassRegistry &);
 void initializeARMLoadStoreOptPass(PassRegistry &);
----------------
dmgreen wrote:
> samtebbs wrote:
> > dmgreen wrote:
> > > Add one of these for the new pass, and call it in LLVMInitializeARMTarget.
> > What is this needed for? It works as-is.
> It's for when the pass has dependencies upon other passes. My understanding is that If this pass needs to know about things like MachineLoop or dominator trees (which is likely in the long run), it will need to have an initialize method that gets called in the right places.
Sounds good, I'll change that.


================
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;
----------------
SjoerdMeijer wrote:
> 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.
> 
>  
That's a good idea. I can use machine loop info to get some more generic information about the loop structure.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:36
+
+static RegisterPass<ARMBlockPlacement> X(DEBUG_TYPE, "ARM block placement",
+                                         false, false);
----------------
dmgreen wrote:
> samtebbs wrote:
> > dmgreen wrote:
> > > I've not seen many things use RegisterPass before. Can this use the `INITIALIZE_PASS` like most other passes do?
> > The skeleton I followed is for the new pass manager.
> RegisterPass doesn't seem to be the new pass manager (and the backend does not support the new pass manager yet). It seems very old and isn't used in a lot of other places.
Oh I see, I followed the pass tutorial for LLVM 12 so this should be up to date, but I can change things if that would be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385