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

Thu Mar 19 09:11:06 PDT 2020

PaoloS marked 5 inline comments as done.
PaoloS added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:86
+
+class RVBInstImm5<bits<7> funct7, bits<3> funct3, RISCVOpcode opcode,
+                    string opcodestr>
----------------
simoncook wrote:
> 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)?
Well, it is used by immediate instructions, so I think and indication of that should appear in the name. I wouldn't base it on RVInstR if that's what you meant. Anyway, in line with what you suggested about RVInstR I could call it ALU_ri_32, but ALU_ri uses a 12 bits immediate field, unlike the instructions using RVBInstImm5 that only use 5 bits immediates. So it's not just a matter of opcode but also of different fields. I could call it RVBInstShift32, as some of the 32 bit counterparts of such instructions use RVBInstShift. The problem is that instructions like GREVIW and GORCIW, or SBSETIW, SBCLRIW, SBINVIW have little to do with shifts. While SLOIW and SROIW have. At the time RVBInstImm5 seemed a good way to express the main difference of such template: the immediate operand bit width. I'll think about better names.

================
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),
----------------
simoncook wrote:
> 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
I could relate to ALU_rr with the name ALU_rr_32 or ALU_rr_w, just to specify the difference.

================
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:
> 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.
I agree. The explicit encoding would avoid some confusion here, even though the fact that this instruction uses the number OPC_OP_IMM for its opcode raises the clash between its immediate format and its "not immediate" usage. This might be subject to some adjustments once we move closer to have the encodings ratified.

================
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<[]>;
----------------
PaoloS wrote:
> edward-jones wrote:
> > 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.
> I agree. The explicit encoding would avoid some confusion here, even though the fact that this instruction uses the number OPC_OP_IMM for its opcode raises the clash between its immediate format and its "not immediate" usage. This might be subject to some adjustments once we move closer to have the encodings ratified.
Splitting the immediate field into fields of 7 and 5 fields seems interesting. I mean, I guess it could prove useful in the future. Never the less it can't be applied anyway to all those templates like RVBInstImm5, RVBInstImm6 and RVBInstImm7, as they expect one of the two fields to be used as an immediate operand. So yes, I could do it, but it applies anyway only to the instructions that use those 12 bits only for flags. Do you agree?

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65649