[PATCH] D65649: [RISCV] Add MC encodings and tests of the Bit Manipulation extension

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Submitting the comments I have so far - need to continue going through in more detail.



================
Comment at: llvm/lib/Target/RISCV/RISCV.td:123
+                       "'B' (Bit Manipulation Instructions)",
+                                [FeatureExtZbb,
+                                 FeatureExtZbc,
----------------
Nit: indentation of this list seems arbitrary. I'd have that `[` should be aligned with the first `"` on the line above?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:1
+//===-- RISCVInstrFormatsB.td - RISCV B Instruction Formats --*- tablegen -*-=//
+//
----------------
Doesn't match the filename


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:9
+//
+//  This file describes the RISC-V B extension instruction formats.
+//
----------------
This should probably say something like "This file describes the RISC-V instructions from the standard 'B' Bitmanip extension, version 0.92. This extension has not yet been ratified (meaning LLVM support is experimental)."


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:48
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVBUnary<bits<7> funct7, bits<5> funct5, bits<3> funct3, RISCVOpcode opcode,
+                     string opcodestr>
----------------
You're using a different style here than elsewhere in the backend, where we try to use the basic RISC-V formats as the superclass wherever possible (and where necessary, defining a subclass in RISCVInstrInfo*.td). RVBUnary for instance just seems to be an R-type instruction (albeit, one where the rs2 field is hard coded). Also, for the base instr format class we don't set hasSideEffects/mayLoad/mayStore (these are semantic details that aren't necessarily implied by the isntr format).


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:51
+    : RVInst<(outs GPR:$rd), (ins GPR:$rs1),
+             opcodestr, "$rd, $rs1", [], InstFormatI> {
+  bits<5> rs1;
----------------
Should be InstFormatR?


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

https://reviews.llvm.org/D65649