Page MenuHomePhabricator

[RISCV] Implement the TargetLowering::getRegisterByName hook
ClosedPublic

Authored by luismarques on Oct 17 2019, 12:29 PM.

Details

Summary

The hook should work for any RISC-V register. Non-allocatable registers do not need to be reserved, for the remaining the hook will only succeed if you pass clang the -ffixed-xX flag. This builds upon D67185, which currently only allows reserving GPRs.

Diff Detail

Event Timeline

luismarques created this revision.Oct 17 2019, 12:29 PM

NFC code tweak.

Nice. This is looking good! One correctness issue, and one low priority issue.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2902

I think you need to be using the Type information in VT to check the register we're returning has the expected width/type

For instance, @llvm.register_read.i32("sp") on RISC-V 64 should absolutely fail. The same @llvm.register_read.i64("sp") should succeed on 64-bit RISC-V. I don't know if this test is done by the register_read intrinsic lowering or has to be done in this code.

llvm/lib/Target/RISCV/RISCVISelLowering.h
150

Low Priority: It would be nice to have a docstring that explains the implementation in RISC-V (including that this is used by @llvm.read_register.<ty>()/@llvm.write_register.<ty>() intrinsics).

luismarques marked 2 inline comments as done.Oct 18 2019, 5:48 AM
luismarques added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2902

If the type doesn't match the lowering will fail, complaining about not knowing how to promote READ_REGISTER. The hook is never reached, so it's not possible to do such check at the hook.

luismarques marked an inline comment as done.
  • Add method documentation comment;
  • Tweak test formatting and var naming.
  • Tweak method doc.
luismarques marked an inline comment as done.Oct 18 2019, 6:38 AM
luismarques edited the summary of this revision. (Show Details)
  • Use getReservedRegs instead of hardcoding the reserved registers.
  • Update summary.

Please can you test using this with the register named fp. This is another alternate name for x8 (aka s0) which is supported in some places, and should be supported here. I'm not sure if MatchRegisterAltName will pick it up. I think x8 is the only register with three names (the rest all only have two).

Tweaks the get-register-reserve.ll test, including addressing @lenary's concern of testing that the fp register name is properly accepted.

lenary accepted this revision.Oct 24 2019, 8:34 AM

LGTM

This revision is now accepted and ready to land.Oct 24 2019, 8:34 AM

Rebase, update test that had not be incorrectly updated.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.
thakis added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2899

This seems a bit strange to me from a dependency point of view. GenAsmMatcher conceptually belongs to RISCVAsmParser, but RISCVCodeGen doesn't depend on RISCVAsmParser. I believe other targets don't include GenAsmMatcher.inc from their codegen libraries (except aarch64, but it's strange there too -- added in D56305, +phosek).

lenary added inline comments.Nov 4 2019, 9:43 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2899

It seems reasonable to use the canonical register name matching, here, rather than a big duplicated StringSwitch. Is there a more reasonable way of avoiding duplication but still using the canonical register information from tablegen?