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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/CodeGen/CalcSpillWeights.cpp:227
+    // not spillable.
+    if (MI->isTerminator() && MI->definesRegister(LI.reg()) &&
+        TII.isUnspillableTerminator(MI)) {
----------------
mtrofin wrote:
> SjoerdMeijer wrote:
> > dmgreen wrote:
> > > 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.
> > Not a big deal, whatever you prefer.
> from a readability perspective, isUnspillableTerminator suggests "isTerminator && isUnspillable".
> 
> How about having isUnspillableTerminator not virtual and doing this:
> 
> return isTerminator() && isUnspillableTerminatorImpl() 
> 
> where isUnspillableTerminatorImpl is protected and virtual?
Sure, sounds good to me.


================
Comment at: llvm/lib/CodeGen/PHIElimination.cpp:452
+             "Expected operand 0 to be a reg def!");
+      SrcRegDef->getOperand(0).setReg(IncomingReg);
+      continue;
----------------
bjope wrote:
> This is only valid if there are no other uses of SrcReg (no anywhere else, and not in the current MBB that we are eliminating PHI nodes inside, such as another PHI node in MBB).
> 
> Haven't analyzed it more closely than that. But I don't know if it can be generally guaranteed that the def in an UnspillableTerminator only has a single use. Is such a rule guaranteed here (or even feasible)?
> 
Yeah. We are being very careful!  We are only using these for hardware loop instructions that we control the insertion of and are careful to only do the transform to produce the instruction (t2LoopEndDec in this case) when it has a single remaining user. CheckUsers in MVETPAndVPTOptimisations::MergeLoopEnd does that.

That's why this is very opt-in and it's trying to be careful to say "Use with care".


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

https://reviews.llvm.org/D91358