[PATCH] D141672: [RISCV] Support vector crypto extension ISA string and assembly

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

@reames and I recently talked and we agreed that vector crypto is far enough along that we'd like to get something committed to LLVM in experimental state. We can do iterative refinement if the spec continues to change. I guess the question is whether we should update to the most recent now or land this first.



================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:495
+                       "'Zvknhb' (Vector SHA-2. (SHA-256 and SHA-512))",
+		       [FeatureStdExtZvknha]>;
+def HasStdExtZvknhaOrZvknhb  : Predicate<"Subtarget->hasStdExtZvknha() || Subtarget->hasStdExtZvknhb()">,
----------------
Indent this to line up with `"'Zvknhb'` on the previous line


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:524
+def HasStdExtZvksh : Predicate<"Subtarget->hasStdExtZvksh()">,
+                                AssemblerPredicate<(all_of FeatureStdExtZvksh),
+                                "'Zvksh' (SM3 Hash Function Instructions.)">;
----------------
This is indented 1 space too far.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:10
+// This file describes the RISC-V instructions from the standard 'Zvk',
+// Vector Cryptography Instructions extension, version 0.1.
+//
----------------
Is this version out of date?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:15
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+multiclass VALU_IV_V_X_VCLMUL<string opcodestr, bits<6> funct6> {
+  def V  : VALUVV<funct6, OPMVV, opcodestr # "." # "vv">,
----------------
Can we name this `VCLMUL_MV_V_X`. That make it consistent with the similar VMUL_MV_V_X, VALU_MV_V_X, etc.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:45
+
+multiclass VALU_IV_V_X_I_VROR<string opcodestr, bits<6> funct6,
+                              Operand optype = uimm6, string vw = "v">
----------------
VROR_IV_V_X_I seems like a more consistent name.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:74
+
+multiclass PALUVvVs2NoVm<bits<6> funct6_vv, bits<6> funct6_vs, bits<5> vs1,
+                         RISCVVFormat opv, string opcodestr> {
----------------
I think this should be something like `VAES_MV_V_S` based on how the other multiclasses are named.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:82
+// to customize one for them.
+class PALUVI_CUSTOM<bits<6> funct6, string opcodestr, Operand optype>
+    : VALUVINoVm<funct6, opcodestr, optype> {
----------------
Maybe this should be something like VAESKF_MV_I?


================
Comment at: llvm/test/MC/RISCV/rvv/zvkb.s:14
+# CHECK-ENCODING: [0x57,0x05,0x94,0x04]
+# CHECK-ERROR: instruction requires the following: 'Zvkb'
+# CHECK-UNKNOWN: 57 05 94 04   <unknown>
----------------
Add `{{$}}` to the end of the line


================
Comment at: llvm/test/MC/RISCV/rvv/zvknh.s:22
+# CHECK-UNKNOWN: 77 25 94 b6   <unknown>
+# CHECK-ERROR: instruction requires the following: 'Zvknha' (Vector SHA-2. (SHA-256 only)) or 'Zvknhb' (Vector SHA-2. (SHA-256 and SHA-512))
+
----------------
Add `{{$}}` to the end of the line to make sure it checks the whole line. Same for all similar lines in all tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141672