[PATCH] D92174: [VE] Optimize emitSPAdjustment function

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 added inline comments.


================
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)
----------------
simoll wrote:
> 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.
I think that way, creating such a function, also.  However, isn't it more generic and resuable that a function uses createVirtualRegister inside?

I have no idea who want to use a function with clobber register this time.  Anyway, I'll make such a function if I find other use of emitAddImmWithClobber.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92174