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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
