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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

Hm, having SHA-2 acceleration but not SHA-3 in 2021 is a bit surprising to me



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:506
   }
+  bool isUImm2() const {
+    int64_t Imm;
----------------
Blank line before


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:522
+    return IsConstantImm && isUInt<4>(Imm) && VK == RISCVMCExpr::VK_RISCV_None;
+  }
   bool isUImm5() const {
----------------
Blank line after


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:196
+                           "'K' (Scalar Cryptography Instructions)">;
+                           
 def Feature64Bit
----------------
Whitespace


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:19
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+    // op rd, rs1
+    class RVKUnary<bits<7> funct7, bits<5> funct5, bits<3> funct3, string opcodestr>
----------------
The formatting for this file is (a) messy (not consistent with itself) and (b) does not match the other extensions. Please reformat the entire thing to conform with the existing style.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:45
+        : RVInstI<funct3, OPC_OP_IMM, (outs GPR:$rd), (ins GPR:$rs1, uimm4:$rcon),
+                opcodestr, "$rd, $rs1, $rcon"> {
+        bits<4> rcon;
----------------
Isn't this the round, which is then used to derive the 8-bit round constant, not the round constant itself?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:56
+let Predicates = [HasStdExtK] in {
+    // RVKUnary
+    def SHA256SUM0 :    RVKUnary<0b0001000, 0b00000, 0b001, "sha256sum0">, 
----------------
These comments are pointless


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:58
+    def SHA256SUM0 :    RVKUnary<0b0001000, 0b00000, 0b001, "sha256sum0">, 
+                        Sched<[]>;
+    def SHA256SUM1 :    RVKUnary<0b0001000, 0b00001, 0b001, "sha256sum1">, 
----------------
If you don't have scheduling defined yet just don't add `Sched<[]>` at all?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:74
+                        Sched<[]>;
+    // Pseudo Instructions
+    def GETNOISE   :   InstAlias<"getnoise $rd", (CSRRS GPR:$rd, MNOISE.Encoding, X0)>, 
----------------
```Assembler Pseudo Instructions```
to distinguish from LLVM `Pseudo`s.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:75
+    // Pseudo Instructions
+    def GETNOISE   :   InstAlias<"getnoise $rd", (CSRRS GPR:$rd, MNOISE.Encoding, X0)>, 
+                        Sched<[]>;
----------------
Naming aliases is pointless


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:133
+} // Predicates = [HasStdExtK, IsRV64]
\ No newline at end of file

----------------
Should be fixed


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:55
   bool HasStdExtV = false;
+  bool HasStdExtK = false;
   bool HasStdExtZvlsseg = false;
----------------
Be consistent whether it's "V K Zv*" or "V Zv* K" (latter being what you've done in TableGen). I think I prefer grouping all the V stuff together even if technically Z* extensions sort after all the single-letter ones.

Also, you will need to get the canonical order updated to include K at some point, though it seems relatively safe to assume it'll just be tacked on the end, i.e. after V (and N).


================
Comment at: llvm/lib/Target/RISCV/RISCVSystemOperands.td:377
+//===-----------------------------------------------
+// K ext CSRs
+//===-----------------------------------------------
----------------
```Machine Scalar Cryptography CSRs```
? I think these headings come from the ISA manual, but K should be consistent both here and in its spec.


================
Comment at: llvm/test/MC/RISCV/rvk/rv32k.s:1
+# RUN: llvm-mc -triple=riscv32 -show-encoding --mattr=+experimental-k %s \
+# RUN:      | FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
----------------
You should have 3 files:

rv32k-valid.s - Tests the instructions common between RV32K and RV64K, with RUN lines for both

rv32k-only-valid.s - Tests the instructions only present in RV32K

rv64k-valid.s - Tests the additional instructions in RV64K


================
Comment at: llvm/test/MC/RISCV/rvk/rv32k.s:12
+sha256sum0 a0, a1
+# CHECK-INST: sha256sum0 a0, a1
+# CHECK-ENCODING: [0x13,0x95,0x05,0x10]
----------------
CHECK lines come before not after


================
Comment at: llvm/test/MC/RISCV/rvk/rv32k.s:144
+# CHECK-UNKNOWN: 33 80 c2 fa <unknown>
\ No newline at end of file

----------------
Fix


================
Comment at: llvm/test/MC/RISCV/rvk/rv64k.s:126
+# CHECK-UNKNOWN: 13 95 55 31 <unknown>
\ No newline at end of file

----------------
Fix


================
Comment at: llvm/test/MC/RISCV/rvk/rvk-invalid.s:1
+# RUN: not llvm-mc -triple riscv32 -mattr=+experimental-k < %s 2>&1 \
+# RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
----------------
Needs to be testing both RV32 and RV64 including instructions specific to each, similar to what I said for the tests above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98136