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

[Edward Jones via Phabricator via llvm-commits]

edward-jones added a comment.

Looks tidy and a really decent set of tests. I've added a few comments, though they are mainly me agreeing with Simon's points.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:182
+
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+let Predicates = [HasStdExtZbb] in {
----------------
Do these properties need to be specified, or do the values match the values which would be inferred by TableGen anyway (since all the DAG patterns are empty)?


================
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:
> 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?
I would also recommend avoiding the opcode enum values. It implies a connection to the base instruction encodings which is entirely incidental, and the name is also misleading.


================
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<[]>;
----------------
edward-jones wrote:
> simoncook wrote:
> > 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?
> I would also recommend avoiding the opcode enum values. It implies a connection to the base instruction encodings which is entirely incidental, and the name is also misleading.
I also think it is worth matching the manual and splitting up the 12 bit encoding into separate 7 and 5 bit fields.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65649