[PATCH] D23561: [RISCV 4/10] Add basic RISCV{InstrFormats, InstrInfo, RegisterInfo, }.td

[James Y Knight via llvm-commits]

jyknight added inline comments.

================
Comment at: lib/Target/RISCV/RISCVRegisterInfo.td:27
@@ +26,3 @@
+// Integer registers
+let RegAltNameIndices = [ABIRegAltName] in {
+  def X0    : Ri<0, "x0", ["zero"]>, DwarfRegNum<[0]>;
----------------
Are the risc-v dwarf register numbers documented anywhere? (I assume not, just like the relocation types, but thought I'd ask anyways. Maybe you can start a list of Things That Ought To Be Documented...)

================
Comment at: lib/Target/RISCV/RISCVRegisterInfo.td:63
@@ +62,3 @@
+def GPR : RegisterClass<"RISCV", [i64], 64, (add
+  (sequence "X%u", 0, 31)
+)>;
----------------
May want to use a different register allocation sequence. Typically backends put caller-save registers first, then callee save. In RISCV, we probably also want to prefer registers in the range X8-X15, since they're usable by the compressed instructions.

How about putting the registers in the order:
X10-X17 [a0-a7] , X5-X7 [t0-t2], X28-X31 [t3-t6], X8-X9 [s0-s1], X18-X27 [s2-s11], X0-X4 [zero, ra, sp, gp, tp]

That is: caller-save between x10-15, then the remaining caller-save, then callee-save, then specials (which will be reserved, anyways)

================
Comment at: lib/Target/RISCV/RISCVRegisterInfo.td:66
@@ +65,2 @@
+
+def GPR32 : RegisterClass<"RISCV", [i32], 32, (add GPR)>;
----------------
I think using one set of registers for both the 32-bit arch and the 64-bit arch doesn't actually work right -- you rather want separate 32bit and 64bit versions of the registers (perhaps name them X{0..31}_32 and X{0..31}_64 for clarity), to make up the GPR32 and GPR64 register classes.

The SPARC backend actually does something like you have here, but I'm pretty sure its 64-bit support is fairly busted and is the wrong way to do things -- better to look at, perhaps, the PPC backend. Could name the registers X{0..31}_32 and X{0..31}_64.



https://reviews.llvm.org/D23561