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

[Paolo Savini via Phabricator via llvm-commits]

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


================
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),
----------------
PaoloS wrote:
> 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.
Never mind, I'll use ALUW_rr that was already available. Must have missed it.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:388
+let Predicates = [HasStdExtB, HasStdExtC] in {
+  def CNOT : RVBInstC<00, "c.not">, Sched<[]>;
+}
----------------
PaoloS wrote:
> 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.
Correction. Adding any of c.not, c.neg, c.zext.w or c.zext.d creates a decoding conflict with the current encoding of c.addi16sp, c.lui and c.lui (hint) already implemented in the C extension. In the current proposed encoding of the compressed instructions a decoding conflict is due. Since these instructions though are meant to be part of the C extensions, not the B extension I think it's better to leave them out of this patch. Once a definitive encoding will be defined for them they should be added with no problems to the C extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65649