[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:
> Add one of these for the new pass, and call it in LLVMInitializeARMTarget.
What is this needed for? It works as-is.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:36
+
+static RegisterPass<ARMBlockPlacement> X(DEBUG_TYPE, "ARM block placement",
+                                         false, false);
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:65
+      bool CanMove = true;
+      for (auto &MI : Target->terminators()) {
+        if (MI.getOpcode() != ARM::t2WhileLoopStart)
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:105
+           "'To' is expected to be a successor of 'From'");
+    MachineInstr &Terminator = *(--From->terminators().end());
+    if (!Terminator.isUnconditionalBranch()) {
----------------
SjoerdMeijer wrote:
> Can there be multiple terminators?
There can indeed. It's a bit of a misnomer in my opinion. As far as I know an unconditional branch and conditional branch could both be terminators in a block.


================
Comment at: llvm/lib/Target/ARM/ARMTargetMachine.cpp:551
   }));
 
   // Don't optimize barriers at -O0.
----------------
dmgreen wrote:
> Add the pass here I think. That way we will have better size estimates from shrinking T2 instructions (and IT blocks should not be a problem for the pass, as far as I understand).
Good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92385