[PATCH] D91358: [ARM][RegAlloc] Add t2LoopEndDec

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

> This might obviously be a little contentious, so I would like to get opinions from people who know what they are talking about.

The ARM changes look very reasonable to me. To be a bit more precise, yes, this is definitely something we want, so agreed on the motivation and the (ARM) implementation, showing nice codegen changes.

But value producing terminators in regalloc is not my forte, so indeed best if someone approves that part.

Find inline some/mostly nits.



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:351
 
+  /// Return true if the given trerminator MI is not expected to spill. This
+  /// sets the live interval as not spillable and adjusts phi node lowering to
----------------
typo: trerminator


================
Comment at: llvm/lib/CodeGen/CalcSpillWeights.cpp:227
+    // not spillable.
+    if (MI->isTerminator() && MI->definesRegister(LI.reg()) &&
+        TII.isUnspillableTerminator(MI)) {
----------------
Nit: guess you don't really need to check isTerminator as that will be checked in isUnspillableTerminator?


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5451
 
+def t2LoopEndDec :
+  t2PseudoInst<(outs GPRlr:$Rm), (ins GPRlr:$elts, brtarget:$target),
----------------
bikeshedding names: how about `t2LoopDecEnd` to more keep the semantics this is a decrement + end, in that order?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1312
   // TODO: Check for copy chains that really have no effect.
-  SmallPtrSet<MachineInstr*, 2> Uses;
-  RDA->getReachingLocalUses(LoLoop.Dec, MCRegister::from(ARM::LR), Uses);
-  if (Uses.size() > 1 || !Uses.count(LoLoop.End)) {
-    LLVM_DEBUG(dbgs() << "ARM Loops: Unable to remove LoopDec.\n");
-    LoLoop.Revert = true;
+  if (LoLoop.Dec != LoLoop.End) {
+    SmallPtrSet<MachineInstr *, 2> Uses;
----------------
It wasn't immediately clear to me why we need this now.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1375
+void ARMLowOverheadLoops::RevertLoopEndDec(MachineInstr *MI) const {
+  LLVM_DEBUG(dbgs() << "ARM Loops: Reverting to subs, br: " << *MI);
+  MachineBasicBlock *MBB = MI->getParent();
----------------
nit: perhaps an assert that MI is a loop end?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1616
+      bool FlagsAlreadySet = RevertLoopDec(LoLoop.Dec);
+      RevertLoopEnd(LoLoop.End, FlagsAlreadySet);
+    }
----------------
nit: how about `RevertLoopEnd(LoLoop.End, RevertLoopDec(LoLoop.Dec));` to get rid of the curly brackets around the if-else here?


================
Comment at: llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp:39
+static cl::opt<bool>
+MergeEndDec("arm-enable-mergeenddec", cl::Hidden,
+    cl::desc("Enable mergeing Loop End and Dec instructions."),
----------------
nit: `arm-enable-mergeenddec` is a bit difficult to read. Since it is relative long already, how about making it even a bit longer, something like:  `arm-enable-merge-loopenddec`?


================
Comment at: llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp:40
+MergeEndDec("arm-enable-mergeenddec", cl::Hidden,
+    cl::desc("Enable mergeing Loop End and Dec instructions."),
+    cl::init(true));
----------------
typo? mergeing?


================
Comment at: llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp:243
 
-  LoopDec->getOperand(1).setReg(PhiReg);
-  LoopEnd->getOperand(0).setReg(DecReg);
+  // Replace the loop dec into the loop end as a single instruction.
+  MachineInstrBuilder MI =
----------------
nit: 

  Replace the loop dec into the loop end as a single instruction
->
  Replace the loop dec and loop end as a single instruction

or something similar...


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

https://reviews.llvm.org/D91358