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

[Simon Cook via Phabricator via llvm-commits]

simoncook added a comment.

This is starting to look good. I've checked all the encodings, and other than c.zext.w having the wrong value all the encodings are right.

One thing I would consider is to make reviewing/future changes easier, I would define all the instructions in the order they appear in the Chapter 2.11 Opcode Encodings tables. This way, it will be easier to see for future revisions what has changed and make sure the encodings are up to date. This might end up with a few more `let Predicates = ` directives, but we can rearrange and tidy this up when it is all ratified.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:86
+
+class RVBInstImm5<bits<7> funct7, bits<3> funct3, RISCVOpcode opcode,
+                    string opcodestr>
----------------
It seems like this is just RVInstR but with a opcode flag, but its only used with OPC_OP_IMM_32, can this just assume that opcode type and have a better name (assuming the opcode type makes sense)?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:116
+
+class RVBInstR<bits<7> funct7, bits<3> funct3, string opcodestr>
+    : RVInstR<funct7, funct3, OPC_OP_32, (outs GPR:$rd),
----------------
This looks like it mostly used for just the 'W' variants of instructions, in that case is this the most appropriate name if the opcode type is OPC_OP_32 by default? It seems for others you're doing the right thing and using ALU_rr as the class you derive from


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:118
+    : RVInstR<funct7, funct3, OPC_OP_32, (outs GPR:$rd),
+              (ins GPR:$rs1, GPR:$rs2), opcodestr, "$rd, $rs1, $rs2"> {}
+
----------------
remove `{}`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:184
+let Predicates = [HasStdExtZbb] in {
+  def CLZ  : RVBInstFunct12<0b011000000000, 0b001, OPC_OP_IMM, "clz">, Sched<[]>;
+  def CTZ  : RVBInstFunct12<0b011000000001, 0b001, OPC_OP_IMM, "ctz">, Sched<[]>;
----------------
I don't know if splitting up the 12-bit immediate into a 7 and a 5 might make future maintenance of this easier, given that that's how its expressed int he manual?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:184
+let Predicates = [HasStdExtZbb] in {
+  def CLZ  : RVBInstFunct12<0b011000000000, 0b001, OPC_OP_IMM, "clz">, Sched<[]>;
+  def CTZ  : RVBInstFunct12<0b011000000001, 0b001, OPC_OP_IMM, "ctz">, Sched<[]>;
----------------
simoncook wrote:
> I don't know if splitting up the 12-bit immediate into a 7 and a 5 might make future maintenance of this easier, given that that's how its expressed int he manual?
I'm also unsure on the use of the opcode enum when it represents the wrong thing, I don't know if this causes some confusion, but we can see if anyone else has any comments here?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:388
+let Predicates = [HasStdExtB, HasStdExtC] in {
+  def CNOT : RVBInstC<00, "c.not">, Sched<[]>;
+}
----------------
This block is missing C.NEG with funct2 `01`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:392
+let Predicates = [HasStdExtB, HasStdExtC, IsRV64] in {
+  def CZESTW : RVBInstC<01, "c.zest.w">, Sched<[]>;
+}
----------------
c.zext.w should have `10` as its funct2. Also zest -> zext


================
Comment at: llvm/test/MC/RISCV/rv64bc-valid.s:13
+# CHECK-ASM: encoding: [0xc0,0x6a]
+c.zest.w t0
----------------
zest -> zext


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65649