This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add clarifying asserts to getFrameIndexReference [NFC]
ClosedPublic

Authored by reames on May 20 2022, 1:12 PM.

Details

Summary

This reorganizes comments slightly (to make the key difference between two big blocks more obvious), and adds some clarifying asserts. This is motivated by me trying to wrap my head around the code enough to usefully review D125787

Diff Detail

Event Timeline

reames created this revision.May 20 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 1:12 PM
reames requested review of this revision.May 20 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 1:12 PM
jrtc27 added inline comments.May 20 2022, 1:26 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
747

The whole point of getFrameRegister is to avoid hard-coding, and this would be a pain for us downstream in CHERI LLVM where pure-capability code uses capability register C8 not the X8 integer sub-register. I mean, I'd just delete the assert downstream rather than repeat the condition... so don't like this assert.

reames added inline comments.May 20 2022, 2:16 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
747

Would having a RISCVABI::getFPReg() and using that in the assert make life easier for you? It would still give me the property I want to assert (e.g. we are using FP if hasFP is true), but maybe be a bit easier to merge?

jrtc27 added inline comments.May 20 2022, 2:21 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
747

We already have getFP/SPReg functions in this file that take the subtarget. Using those for this would be fine by me.

reames updated this revision to Diff 431065.May 20 2022, 2:48 PM

Address review comment

StephenFan added inline comments.May 21 2022, 1:29 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
717

Does if present means if the BP exists? But If both realignment and VarSize objects exist, then BP must be present.

747

I also don't like this assertion and IMO we need to avoid this trivial assertion. I call it a trivial assertion since getFrameRegister would return the frame register according to the result of hasFP, this assertion would always succeed unless the compiler generates the wrong code.

reames added inline comments.May 23 2022, 9:49 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
717

Please see the code immediately below this. If you have a suggestion on the comment wording, feel free to suggest something.

747

Unless you can point me to a comment that says that's the document semantics of getFrameRegister, I disagree this is a trivial assert.

Going even further, it's not clear to me that the getFrameRegister actually even implements the contract its supposed to. Per the header comment in TargetRegisterInfo.h, it seems this should be returning BP in the cases that's the register we index off. However, that's *definitely* a separate patch.

StephenFan added inline comments.May 23 2022, 11:24 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
717

Oh, I see. Now it's ok. Thanks

723

Remove the curly bracket?

reames added inline comments.May 24 2022, 11:01 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
723

We generally prefer consistency between if and else. The else block requires the bracket.

reames updated this revision to Diff 431729.May 24 2022, 11:05 AM

Rebase, and drop frame register asserts. I'll move those into a separate commit, but they're slowing down the more useful part of the patch.

frasercrmck accepted this revision.May 24 2022, 9:46 PM

LGTM, thanks. I think this is easier to understand.

This revision is now accepted and ready to land.May 24 2022, 9:46 PM
StephenFan added inline comments.May 24 2022, 10:14 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
723

Makes sense to me.

This revision was landed with ongoing or failed builds.May 25 2022, 7:59 AM
This revision was automatically updated to reflect the committed changes.