Page MenuHomePhabricator

[VE] Optimize emitSPAdjustment function
ClosedPublic

Authored by kaz7 on Nov 26 2020, 5:33 AM.

Details

Summary

Optimize emitSPAdjustment function to generate as small as possible
instructions to adjust SP.

Diff Detail

Event Timeline

kaz7 created this revision.Nov 26 2020, 5:33 AM
kaz7 requested review of this revision.Nov 26 2020, 5:33 AM
simoll added inline comments.Nov 26 2020, 6:54 AM
llvm/lib/Target/VE/VEFrameLowering.cpp
231

typo: Nothing

232–243

Consider factoring this into a separate "LoadImm" function.

kaz7 added inline comments.Nov 27 2020, 3:37 AM
llvm/lib/Target/VE/VEFrameLowering.cpp
231

Thanks.

232–243

This is "addImmediate" to SP and need to specify clobber register also. I think it is not general instructions as you expected.

kaz7 updated this revision to Diff 308011.Nov 27 2020, 3:37 AM

Correct typo in a comment

simoll added inline comments.Nov 27 2020, 5:04 AM
llvm/lib/Target/VE/VEFrameLowering.cpp
232–243

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.

kaz7 added inline comments.Nov 27 2020, 5:15 AM
llvm/lib/Target/VE/VEFrameLowering.cpp
232–243

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.

simoll accepted this revision.Nov 27 2020, 6:15 AM

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

llvm/lib/Target/VE/VEFrameLowering.cpp
232–243

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.

This revision is now accepted and ready to land.Nov 27 2020, 6:15 AM
kaz7 added inline comments.Nov 27 2020, 2:35 PM
llvm/lib/Target/VE/VEFrameLowering.cpp
232–243

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.

This revision was landed with ongoing or failed builds.Nov 27 2020, 3:06 PM
This revision was automatically updated to reflect the committed changes.