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

[Alex Bradbury via llvm-commits]

asb added a subscriber: hfinkel.

================
Comment at: lib/Target/RISCV/RISCV.td:23
@@ +22,3 @@
+
+def : ProcessorModel<"RV64I", NoSchedModel, [Feature64Bit]>;
+
----------------
theraven wrote:
> Should these not be `Processor`, rather than `ProcessorModel`.  I was under the impression that `Processor` is intended for generic families, whereas `ProcessorModel` was intended for specific microarchitectures.
> 
> It would be nice to have Rocket as a specific `ProcessorModel` here.  It may be in a later review, but we should probably also have `SubtargetFeature` flags for floating point, atomics, and so on here too.
I think actually `ProcessorModel` is preferred as it exposes the more modern `SchedMachineModel` rather than the legacy `ProcessorItineraries`. At least X86, AArch64, and Lanai describe generic `ProcessorModel`s. See this email from @hfinkel, who can perhaps confirm my interpretation <http://lists.llvm.org/pipermail/llvm-dev/2015-November/092214.html>. It looks like an uppercase `ProcessorModel` name is uncommon though, so I will change these to `generic-rv32` and `generic-rv64`.

Having a Rocket `ProcessorModel` and scheduling model is definitely part of the plan, and later patches will add appropriate `SubtargetFeature` flags.

================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:143
@@ +142,3 @@
+{
+  bits<21> imm20;
+  bits<5> rd;
----------------
jordy.potman.lists wrote:
> Shouldn't this now be
> ```
> bits<20> imm20;
> ```
> ?
It should be - thanks!


https://reviews.llvm.org/D23561