[PATCH] D69130: [RISCV] Implement the TargetLowering::getRegisterByName hook

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

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



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2894
+Register
+RISCVTargetLowering::getRegisterByName(const char *RegName, EVT VT,
+                                       const MachineFunction &MF) const {
----------------
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.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:150
 
+  Register getRegisterByName(const char *RegName, EVT VT,
+                             const MachineFunction &MF) const override;
----------------
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69130/new/

https://reviews.llvm.org/D69130