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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:18-19
+
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+    class RVKUnary<bits<7> funct7, bits<5> funct5, bits<3> funct3, string opcodestr>
+        : RVInstR<funct7, funct3, OPC_OP_IMM, (outs GPR:$rd), (ins GPR:$rs1), opcodestr, "$rd, $rs1">{
----------------
Indenting is still off:
- 2 spaces not 4
- we don't typically indent inside `let`s


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:58
+let Predicates = [HasStdExtK] in {
+    def SHA256SUM0 :    RVKUnary<0b0001000, 0b00000, 0b001, "sha256sum0">, 
+                        Sched<[]>;
----------------
Only one space after the colon


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:66-69
+    def SM3P0      :    RVKUnary<0b0001000, 0b01000, 0b001, "sm3p0">, 
+                        Sched<[]>;
+    def SM3P1      :    RVKUnary<0b0001000, 0b01001, 0b001, "sm3p1">, 
+                        Sched<[]>;
----------------
I think it'd be neater to make this a separate "group" (i.e. add a blank line between SHA256SIG1 and SM3P0, and then not align the new group with the sha256 ones), as that'll cut down on the number of spaces


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:97-104
+    def AES32ESMI  :    RVKByteSelect<0b11011, "aes32esmi">,
+                        Sched<[]>;
+    def AES32ESI  :     RVKByteSelect<0b11001, "aes32esi">,
+                        Sched<[]>;
+    def AES32DSMI  :    RVKByteSelect<0b11111, "aes32dsmi">,
+                        Sched<[]>;
+    def AES32DSI  :     RVKByteSelect<0b11101, "aes32dsi">,
----------------
Line things up properly if you're going to go to the trouble of doing it


================
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;
----------------
VincentWu wrote:
> jrtc27 wrote:
> > Isn't this the round, which is then used to derive the 8-bit round constant, not the round constant itself?
> I'm a little confused about that. So do you mean number rcon is a kind of offset or something？and we need an extra base address?
No. Wikipedia has details (https://en.wikipedia.org/wiki/AES_key_schedule#Round_constants), but basically the the round number counts up 1 by 1, but the actual round constant is a function of the round number (either implemented with a simple table or the slightly more general formula). I've filed https://github.com/riscv/riscv-crypto/issues/82 about this, but given that's what the spec currently calls it I think it's fine to call it `rcon` in LLVM.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoK.td:58
+    def SHA256SUM0 :    RVKUnary<0b0001000, 0b00000, 0b001, "sha256sum0">, 
+                        Sched<[]>;
+    def SHA256SUM1 :    RVKUnary<0b0001000, 0b00001, 0b001, "sha256sum1">, 
----------------
VincentWu wrote:
> jrtc27 wrote:
> > If you don't have scheduling defined yet just don't add `Sched<[]>` at all?
> yes, but there will be something wrong whing building if I just remove the `Sched<[]>`. and we found that the RISCVInstrInfoB.td which is already merged also put an empty `Sched<[]>` there. So we put it there too.
That shouldn't be the case if you add HasStdExtK to UnsupportedFeatures for the models


================
Comment at: llvm/test/MC/RISCV/rvk/rv32k-valid.s:1
+# RUN: llvm-mc -triple=riscv32 -show-encoding --mattr=+experimental-k %s \
+# RUN:      | FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
----------------
These files probably don't need to be in their own subdirectory. RVV has one, but that's only because it's such a huge extension, whereas this is a much smaller extension so can be like how we're testing all the others.


================
Comment at: llvm/test/MC/RISCV/rvk/rvk32-invalid.s:1
+# RUN: not llvm-mc -triple=riscv32 -mattr=+experimental-k < %s 2>&1 \
+# RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
----------------
File should be rv32k- not rvk32 (same for the file below)


================
Comment at: llvm/test/MC/RISCV/rvk/rvk32-invalid.s:2
+# RUN: not llvm-mc -triple=riscv32 -mattr=+experimental-k < %s 2>&1 \
+# RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
+
----------------
Need a riscv64 RUN line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98136