This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Store/restore RISCVMachineFunctionInfo into MIR YAML file
ClosedPublic

Authored by kito-cheng on Apr 5 2022, 9:08 PM.

Details

Summary

RISCVMachineFunctionInfo has some fields like VarArgsFrameIndex and
VarArgsSaveSize are calculated at ISel lowering stage, those info are
not contained in MIR files, that cause test cases rely on those field
can't not reproduce correctly by MIR dump files.

This patch adding the MIR read/write for those fields.

Diff Detail

Event Timeline

kito-cheng created this revision.Apr 5 2022, 9:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 9:08 PM
kito-cheng requested review of this revision.Apr 5 2022, 9:08 PM
frasercrmck added inline comments.Apr 6 2022, 1:05 AM
llvm/test/CodeGen/RISCV/machine-function-info.mir
97 ↗(On Diff #420698)

I guess we should be checking that machineFunctionInfo is serialized back out again successfully? Seems like update_mir_test_checks.py isn't doing that for us.

craig.topper added inline comments.Apr 6 2022, 9:07 AM
llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h
29

Why is MoveF64FrameIndex in here but never referenced anywhere else.

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
241

What makes this need a reinterpret_cast instead of a static_cast? I see that other targets are also using reinterpret_cast but I don't understand them either.

arsenm added a subscriber: arsenm.Apr 6 2022, 2:57 PM
arsenm added inline comments.
llvm/test/CodeGen/RISCV/machine-function-info.mir
2 ↗(On Diff #420698)

Would be a good idea to have a round trip run line to check printing and parsing

137 ↗(On Diff #420698)

MIR print/parser tests should go under test/CodeGen/MIR/RISCV, not test/CodeGen/RISCV

craig.topper added inline comments.Apr 6 2022, 7:49 PM
llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
241

I pushed a fix to use static_cast on the other targets.

kito-cheng updated this revision to Diff 421074.Apr 6 2022, 8:37 PM

Changes:

  • Checking machineFunctionInfo.
  • Remove MoveF64FrameIndex.
  • Use static_cast rather than reinterpret_cast.
  • Move test case to test/CodeGen/MIR/RISCV.
kito-cheng marked 5 inline comments as done.Apr 6 2022, 8:41 PM
kito-cheng added inline comments.
llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h
29

I was try to serialize that, but I realize that is not necessary soon, however I forgot to remove that...

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
241

Thanks, actually that's copy and paste from AArch64...:P

llvm/test/CodeGen/RISCV/machine-function-info.mir
97 ↗(On Diff #420698)

Thanks, check added!

137 ↗(On Diff #420698)

Moved :)

frasercrmck accepted this revision.Apr 7 2022, 2:24 AM

LGTM other than a couple of issues to fix up.

llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.cpp
10

This comment's incorrect, isn't it? Something like this, I'd imagine:

// This file declares RISCV-specific per-machine-function information.
llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
242

I don't see much value in creating MF here since we only use it once. I see it's a copy from AArch64. Just PFS.MF.getInfo.... would do?

llvm/test/CodeGen/MIR/RISCV/machine-function-info.mir
2

I think we can merge these two RUN lines: they're less than 80 cols combined.

This revision is now accepted and ready to land.Apr 7 2022, 2:24 AM
This revision was landed with ongoing or failed builds.Apr 7 2022, 8:55 PM
This revision was automatically updated to reflect the committed changes.
kito-cheng marked 3 inline comments as done.