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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for taking a look.



================
Comment at: llvm/lib/CodeGen/CalcSpillWeights.cpp:227
+    // not spillable.
+    if (MI->isTerminator() && MI->definesRegister(LI.reg()) &&
+        TII.isUnspillableTerminator(MI)) {
----------------
SjoerdMeijer wrote:
> Nit: guess you don't really need to check isTerminator as that will be checked in isUnspillableTerminator?
I was trying to make it explicit, that it needed to be a terminator that defined the value and this shouldn't be used for anything else.

I can change that if you think it's better to remove it. It is currently just trying to be conservative.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5451
 
+def t2LoopEndDec :
+  t2PseudoInst<(outs GPRlr:$Rm), (ins GPRlr:$elts, brtarget:$target),
----------------
SjoerdMeijer wrote:
> bikeshedding names: how about `t2LoopDecEnd` to more keep the semantics this is a decrement + end, in that order?
Yeah, It does both together. I have no strong opinion on the order in the name. I guess I chose t2LoopEndDec as I felt that the major thing it did was behave as a loop end (like an LE instruction). The Dec somewhat of a side effect of that.

Can change it if you think it's better the other way. Let me know.


================
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;
----------------
SjoerdMeijer wrote:
> It wasn't immediately clear to me why we need this now.
I updated the comment.


================
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."),
----------------
SjoerdMeijer wrote:
> 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`?
Thanks. This did feel like a particularly ugly option name.


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

https://reviews.llvm.org/D91358