[PATCH] D92174: [VE] Optimize emitSPAdjustment function

[Simon Moll via Phabricator via llvm-commits]

simoll accepted this revision.
simoll added a comment.
This revision is now accepted and ready to land.

It's about making the code more reusable - this is not a stopper for this patch, however.



================
Comment at: llvm/lib/Target/VE/VEFrameLowering.cpp:232-243
+  } else if (isInt<7>(NumBytes)) {
+    // adds.l %s11, NumBytes at lo, %s11
     BuildMI(MBB, MBBI, DL, TII.get(VE::ADDSLri), VE::SX11)
         .addReg(VE::SX11)
         .addImm(NumBytes);
-    return;
+  } else if (isInt<32>(NumBytes)) {
+    // lea %s11, NumBytes at lo(, %s11)
----------------
kaz7 wrote:
> simoll wrote:
> > kaz7 wrote:
> > > simoll wrote:
> > > > Consider factoring this into a separate "`LoadImm`" function.
> > > This is "addImmediate" to SP and need to specify clobber register also.  I think it is not general instructions as you expected.
> > So, you could name it `emitAddImm(Reg, Imm)`. The reason i was asking you to consider this, is that putting this into its own function with a telling name would document what you are doing here and make the functionality reuseable. I imagine there may be other places where we need to emit code to increment a register by a constant.
> As I said, it requires clobber register like S13 in this case.  I simply don't understand what you really want.  I already named like `emitSPAdjustment`.  Should I change this name to `emitAddSPImm` or something?  Or should I make a new function to handle only `isInt<7>` and `isInt<32>` cases?  Or should I make a new function to handle all cases?  If this function generates really generic instructions, I agree with you.  However, this isn't IMHO.
You could put all of the cases in a function called:
 
   bool emitAddImm(Register TheReg, Imm TheValue, Register ClobberReg)

That function would fail if the ClobberReg is invalid but required. It returns true if the ClobberReg is valid and had to be used to load the immediate. Else, it returns false and the ClobberReg was not necessary to load the immediate. This would be a reusable function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92174