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

Mircea Trofin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 25 09:46:30 PST 2020

mtrofin added inline comments.

Comment at: llvm/lib/CodeGen/CalcSpillWeights.cpp:227
+    // not spillable.
+    if (MI->isTerminator() && MI->definesRegister(LI.reg()) &&
+        TII.isUnspillableTerminator(MI)) {
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?

Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1375
+  MIB.addImm(ARMCC::AL);
+  MIB.addReg(0);
+  MIB.addReg(ARM::CPSR);
Nit: addReg(MCRegister::NoRegister) is probably more readable.



More information about the llvm-commits mailing list