[PATCH] D98136: [RISCV][RFC] Initially support the K-extension instructions on the LLVM MC layer

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1254
+  case Match_InvalidRnumArg: {
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 4) - 6);
+  }
----------------
`(1 << 4) - 6` -> `10`.   There's no reason to write a more complex expression. It makes more sense for things like Match_InvalidUImm2 to show the connection between the name of the error and the integer values. That doesn't apply here.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:1
+//===-- RISCVInstrInfoK.td - RISC-V 'K' instructions -------*- tablegen -*-===//
+//
----------------
K -> Zk in both places


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:9
+//
+// This file describes the RISC-V instructions from the standard 'K',
+// Cryptography Instructions extension, version 1.0.
----------------
K -> Zk


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:24
+
+def rnum : Operand<XLenVT>, ImmLeaf<XLenVT, [{return isInt<4>(Imm);}]> {
+  let ParserMatchClass = RnumArg;
----------------
This ImmLeaf should check 0-10. Alternatively it can be removed until CodeGen support is added.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:28
+  let DecoderMethod = "decodeUImmOperand<4>";
+  let OperandType = "OPERAND_UIMM4";
+  let OperandNamespace = "RISCVOp";
----------------
It's not a UIMM4. It should have its own OPERAND type and the range check for it in X86InstrInfo.cpp should check 0-10.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket.td:20
   let CompleteModel = false;
-  let UnsupportedFeatures = [HasStdExtV, HasStdExtZvlsseg];
+  let UnsupportedFeatures = [HasStdExtZknd, HasStdExtZkne, HasStdExtZknh,
+                             HasStdExtZksed, HasStdExtZksh, HasStdExtZkr,
----------------
This line needs to rebased I think. I'll also be removed Zvlsseg later today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98136