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

[Tim Northover via llvm-commits]

t.p.northover added a subscriber: t.p.northover.

================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:33
@@ +32,3 @@
+
+class FR<bits<7> opcode, bits<3> funct3, bits<7> funct7, dag outs, dag ins,
+         string asmstr, list<dag> pattern> : RISCVInst<outs, ins, asmstr, pattern>
----------------
Nit: since the spec seems to list instructions from bit 31 down to 0 (as do TableGen 0bxxxx literals), definitions might read more naturally if you make the order "funct7, funct3, opcode".

================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:97
@@ +96,3 @@
+{
+  bits<13> imm13;
+  bits<5> rs1;
----------------
This immediate doesn't appear to have a bit 0 in the spec (it fits into exactly the same fields as imm12 does).

So what we're really deciding here is what the immediate in the MCInst representation of "BEQ #x" should be. Valid answers are "x" and "the bits representing how x is encoded", with newer targets tending to favour the latter.

This is mostly because it makes it harder by design to construct an invalid MCInst when you're doing it manually. Other than that, it's just punting complexity around the various MC layer components:
  
| MC component | with "x" | with bits |
|--|--|--|
| AsmParser | nop | encodes |
| Disassembler | decodes | nop |
| AsmPrinter | nop | decodes |
| Fixup handling | encodes | nop |

Either way "imm13" is probably misleading as a name and it's probably a good idea to be consistent between formats if possible (I note FU has taken the second approach).


https://reviews.llvm.org/D23561