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

Sjoerd Meijer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 23 08:48:44 PST 2020

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 =

  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...



More information about the llvm-commits mailing list