Page MenuHomePhabricator

[RISCV] Reuse and generalize adjustReg from another spot in frame lowering [nfc]
ClosedPublic

Authored by reames on Nov 29 2022, 7:43 AM.

Details

Summary

Change is straight forward. Posted for review for a second set of eyes on the generalization. In particularly, SrcReg being equal to DestReg is new with this caller. Already caught one bug due to that, anyone spot anything else?

Diff Detail

Event Timeline

reames created this revision.Nov 29 2022, 7:43 AM
reames requested review of this revision.Nov 29 2022, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 7:43 AM
frasercrmck accepted this revision.Nov 30 2022, 1:13 AM

LGTM (with nit) - I couldn't find or think of any problems arising from this.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
351

We have RISCVFrameLowering::adjustReg (used elsewhere in this file) which wraps the call to RI.adjustReg. I think we should continue to use use adjustReg (for now) even though they're equivalent.

Personally I'm not a huge fan of having identically-named methods across multiple files/classes. So if you wanted to remove RISCVFrameLowering::adjustReg and make everyone call the RISCVRegisterInfo one directly, I'd support that.

This revision is now accepted and ready to land.Nov 30 2022, 1:13 AM
asb accepted this revision.Nov 30 2022, 3:52 AM

I don't see any issues either. I don't have a strong view about @frasercrmck's nit.

reames added inline comments.Nov 30 2022, 9:37 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
351

This is certainly the end goal, and after this patch and D132839
which just landed, I think I'm close to being able to do that. Might be one or two more cleanup changes first, but that's definitely where we're heading.

This revision was landed with ongoing or failed builds.Nov 30 2022, 9:43 AM
This revision was automatically updated to reflect the committed changes.