[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly
Eric Gouriou via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 6 02:44:04 PST 2022
ego added a comment.
I have been working on a patch set to support Zvk. It will take me a few more days to prepare my patches to post them publicly.
Your patches are in a very good shape and a lot of files end up looking pretty similar.
I have a few comments, most minor.
This is my first review on Phabricator, don't hesitate to provide feedback/suggestions on my feedback.
================
Comment at: clang/test/Preprocessor/riscv-target-features.c:486
+// RUN: -march=rv32izvkb0p1 -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-ZVKB-EXT %s
+// RUN: %clang -target riscv64 -menable-experimental-extensions \
----------------
Minor: please keep the Zvk sub-extensions in alphabetical order (zvkb, zvkg, zvknha, zvknhb, zvkns, zvksed, zvksh).
================
Comment at: llvm/docs/RISCVUsage.rst:147
+``experimental-zvkns``, ``experimental-zvknha``, ``experimental-zvknhb``, ``experimental-zvkb``, ``experimental-zvkg``, ``experimental-zvksed``, ``experimental-zvksh``
+ LLVM implements the `0.1 draft specification <https://github.com/riscv/riscv-crypto/releases/download/v20221118/riscv-crypto-spec-vector_20221118.pdf>`_. Note that current vector crypto extension version can be found in: <https://github.com/riscv/riscv-crypto>.
----------------
Minor: please keep the Zvk sub-extensions in alphabetical order (zvkb, zvkg, zvknha, zvknhb, zvkns, zvksed, zvksh).
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:125
+ {"zvknhb", RISCVExtensionVersion{0, 1}},
+ {"zvkb", RISCVExtensionVersion{0, 1}},
+ {"zvkg", RISCVExtensionVersion{0, 1}},
----------------
Minor: please keep the Zvk sub-extensions in alphabetical order (zvkb, zvkg, zvknha, zvknhb, zvkns, zvksed, zvksh).
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:826
{{"zvfh"}, {ImpliedExtsZvfh}},
+ {{"zvkb"}, {ImpliedExtsZve64x}},
+ {{"zvkg"}, {ImpliedExtsZve32x}},
----------------
Can you please point me to where the specification(s) state those implications?
Are those the semantics we want? I know the spec reviewers are keen to ensure that the spec works on vector units with a VLEN smaller than the element group. Is there a requirement that VLEN is at least as large as the element?
So far I asked all my uses of zvk extensions to also declare v (or Zve) explicitly. I believe there is a precedent for another extension relying on vector but will need to double-check.
Along the same lines, we'll want to figure what declaring "Zvk" means.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:827
+ {{"zvkb"}, {ImpliedExtsZve64x}},
+ {{"zvkg"}, {ImpliedExtsZve32x}},
+ {{"zvknha"}, {ImpliedExtsZve32x}},
----------------
What is the reasoning between 32 vs 64 for those sub-extensions?
I do not think that extensions using 64bxN element groups are restricted to rv64.
No matter what the end result is, I would welcome some comments explaining the encoded semantics.
================
Comment at: llvm/lib/Target/RISCV/RISCV.td:473
+ AssemblerPredicate<(any_of FeatureStdExtZvknha, FeatureStdExtZvknhb),
+ "'Zvknha' (Vector SHA-2. (SHA-256 only)) "
+ "'Zvknhb' (Vector SHA-2. (SHA-256 and SHA-512))">;
----------------
Please consider adding an "or" in the string.
================
Comment at: llvm/lib/Target/RISCV/RISCV.td:476
+
+def FeatureStdExtZvkb
+ : SubtargetFeature<"experimental-zvkb", "HasStdExtZvkb", "true",
----------------
Minor: please keep the Zvk sub-extensions in alphabetical order (zvkb, zvkg, zvknha, zvknhb, zvkns, zvksed, zvksh).
Note: I just realized the spec lists them in the order used here. So feel free to ignore those suggestions if you'd rather follow the spec ordering (although I may try to nudge Ken towards an alphabetical ordering in the spec).
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:63
+ let Opcode = OPC_OP_P.Value;
+ let Inst{14-12} = 0b010;
+}
----------------
Suggestion: OMPVV.Value;
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:78
+
+class PALUVI_CUSTOM<bits<6> funct6, string opcodestr, Operand optype>
+ : VALUVINoVm<funct6, opcodestr, optype> {
----------------
This deserves a comment about intended usage.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:85
+
+def rnum_0_7 : Operand<XLenVT>, ImmLeaf<XLenVT,
+ [{return (0 <= Imm && Imm <= 7);}]> {
----------------
Note: there is a way to avoid code duplication using classes, and to also cover the similar use in the scalar crypto instructions.
I will post Zvk patches, but the code looks like this:
In RISCVIntrInfo.td:
// Parser match class for Round Number operands
// with defined min/max inclusive bounds.
class RnumParserOperand<int min, int max> : AsmOperandClass {
let Name = "Rnum" # min # "_" # max # "Operand";
let RenderMethod = "addImmOperands";
let DiagnosticType = "InvalidRnum" # min # "_" # max # "Operand";
}
// Oerand class representing cryptographic Round Number operands
// with defined 'min'/'max' inclusive bounds. The 'pred' predicate
// should look like '[{return (Imm >= 0 && Imm <= 7);}]', with 0/7
// replaced by the actual min/max values.
class Rnum<int min, int max, code pred> : Operand<i32>, TImmLeaf<i32, pred> {
let ParserMatchClass = RnumParserOperand<min, max>;
let EncoderMethod = "getImmOpValue";
let DecoderMethod = "decodeUImmOperand<5>";
let OperandType = "OPERAND_RVRNUM" # min # "_" # max;
let OperandNamespace = "RISCVOp";
}
def rnum0_7 : Rnum<0, 7, [{return (Imm >= 0 && Imm <= 7);}]>;
def rnum0_10 : Rnum<0, 10, [{return (Imm >= 0 && Imm <= 10);}]>;
def rnum0_31 : Rnum<0, 31, [{return (Imm >= 0 && Imm <= 31);}]>;
def rnum1_10 : Rnum<1, 10, [{return (Imm >= 1 && Imm <= 10);}]>;
def rnum2_14 : Rnum<2, 14, [{return (Imm >= 2 && Imm <= 14);}]>;
In RISCVIntrInfo.cpp:
// clang-format off
#define CASE_OPERAND_RVRNUM(MIN, MAX) \
case RISCVOp::OPERAND_RVRNUM##MIN##_##MAX: \
Ok = Imm >= MIN && Imm <= MAX; \
break;
CASE_OPERAND_RVRNUM(0, 7)
CASE_OPERAND_RVRNUM(0, 10)
CASE_OPERAND_RVRNUM(0, 31)
CASE_OPERAND_RVRNUM(1, 10)
CASE_OPERAND_RVRNUM(2, 14)
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:110
+let Predicates = [HasStdExtZvkns] in {
+ def VAESDF_VV : PALUVs2NoVm<0b101000, 0b00001, RISCVVFormat<0b010>, "vaesdf.vv">;
+ def VAESDF_VS : PALUVs2NoVm<0b101001, 0b00001, RISCVVFormat<0b010>, "vaesdf.vs">;
----------------
Is this OPMVV.Value? If so, let's make this explict.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:123
+
+let Predicates = [HasStdExtZvkb] in {
+ defm VANDN_V : VALU_IV_V_X_I<"vandn", 0b000001>;
----------------
MInor: let's keep the extensions in alphabetical order.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:139
+ def VSM4K_VI : PALUVINoVm<0b100001, "vsm4k.vi", rnum_0_7>;
+ def VSM4R_VV : PALUVs2NoVm<0b101000, 0b10000, RISCVVFormat<0b010>, "vsm4r.vv">;
+} // Predicates = [HasStdExtZvksed]
----------------
FYI, a .VS variant has just been added to the spec.
================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:100
+ bool HasStdExtZvknhb = false;
+ bool HasStdExtZvkb = false;
+ bool HasStdExtZvkg = false;
----------------
Minor: Another call to keep extensions in alphabetical order, at least within Zvk. (same comment below for the Has... methods)
================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:141
+; RV32ZVKNHA: .attribute 5, "rv32i2p0_zve32x1p0_zvknha0p1_zvl32b1p0"
+; RV32ZVKNHB: .attribute 5, "rv32i2p0_zve32x1p0_zve64x1p0_zvknha0p1_zvknhb0p1_zvl32b1p0_zvl64b1p0"
+; RV32ZVKB: .attribute 5, "rv32i2p0_zve32x1p0_zve64x1p0_zvkb0p1_zvl32b1p0_zvl64b1p0"
----------------
Can you please point me to the logic that transforms a zvknhb into the nha and nhb attributes being emitted?
Note that I may be misunderstanding what this case is checking.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvkb.s:1
+# RUN: llvm-mc -triple=riscv64 -show-encoding --mattr=+experimental-zvkb %s \
+# RUN: | FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
----------------
I *believe* Zvk does not require riscv64, but is also applicable to riscv32.
I am checking with Ken Dockser.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvknhb.s:14
+# CHECK-ENCODING: [0x77,0x25,0x94,0xb6]
+# CHECK-ERROR: instruction requires the following: 'Zvknha' (Vector SHA-2. (SHA-256 only)) 'Zvknhb' (Vector SHA-2. (SHA-256 and SHA-512))
+# CHECK-UNKNOWN: 77 25 94 b6 <unknown>
----------------
Minor: it would be best for the user to have an "or" in that message.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvksed.s:17
+
+vsm4r.vv v10, v9
+# CHECK-INST: vsm4r.vv v10, v9
----------------
Note the very recent addition of vsm4r.vs.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138807/new/
https://reviews.llvm.org/D138807
More information about the llvm-commits
mailing list