[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly
Eric Gouriou via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 16 00:27:39 PST 2022
ego added inline comments.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:827
+ {{"zvkb"}, {ImpliedExtsZve64x}},
+ {{"zvkg"}, {ImpliedExtsZve32x}},
+ {{"zvknha"}, {ImpliedExtsZve32x}},
----------------
craig.topper wrote:
> ego wrote:
> > 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.
> The 32 and 64 refer to the ELEN. Zknhb requires SEW=64 so ELEN must be 64
Thanks Craig, this addresses my previous comment.
We still have to figure what declaring "Zvk" means, but that can wait.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:826
{{"zvfh"}, {ImpliedExtsZvfh}},
+ {{"zvkg"}, {ImpliedExtsZve32x}},
+ {{"zvknha"}, {ImpliedExtsZve32x}},
----------------
Zvkb contains vclmul, which is restricted to SEW=64. I think we can state that it implies ImpliedExtsZve64x.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:826
{{"zvfh"}, {ImpliedExtsZvfh}},
+ {{"zvkg"}, {ImpliedExtsZve32x}},
+ {{"zvknha"}, {ImpliedExtsZve32x}},
----------------
ego wrote:
> Zvkb contains vclmul, which is restricted to SEW=64. I think we can state that it implies ImpliedExtsZve64x.
I believe zvkb is missing here. I think it implies Zve64x due to vclmul/vclmulh which require SEW=64.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:827
+ {{"zvkg"}, {ImpliedExtsZve32x}},
+ {{"zvknha"}, {ImpliedExtsZve32x}},
+ {{"zvknhb"}, {ImpliedExtsZve64x}},
----------------
How does this work? This doesn't seem to be enough,
"ImpliedExtsZve32x" does not expand (recursively) to contain "zve32x", which is necessary to satisfy "HasVector" in checkDependency(), which leads to an error (with some dbgs() statements added in updateImplication()):
> % .. && bin/llvm-lit -v ../llvm/test/CodeGen/RISCV/attributes.ll
> ...
> DBG: --- Entering updateImplication
> DBG: Adding new implied ext >zvknha< => >zvl32b<
> DBG: --- Exiting updateImplication
> LLVM ERROR: zvl*b requires v or zve* extension to also be specified
That's because 'ImpliedExtsZve32x' does not include Zve32x, but only the extensions implied by Zve32x. So we don't end up with Zve32x being defined.
So I *think* that we either want to imply "zve32x", maybe in a ImpliedExtsZvkEW32 (/ ImpliedExtsZvkEW64) to be used by Zvk sub-extensions that imply that a 32b-wide SEW is supported (/64b-wide), or we need to ask users to specify a vector extension when also declaring a Zvk* extension.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:147
def OPC_CUSTOM_2 : RISCVOpcode<"CUSTOM_2", 0b1011011>;
+def OPC_OP_P : RISCVOpcode<"OP_P", 0b1110111>;
def OPC_BRANCH : RISCVOpcode<"BRANCH", 0b1100011>;
----------------
Minor: to maintain numerical ordering, this should appear between OPC_SYSTEM and OPC_CUSTOM_3 (if I got it right).
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:236
+def uimm6 : Operand<XLenVT> {
+ let ParserMatchClass = UImmAsmOperand<6>;
----------------
Minor: let's keep those in increase numerical order, so let's place it before the uimm7 logic, after the uimm5 one.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:101
+ let DecoderMethod = "decodeUImmOperand<5>";
+ let OperandType = "OPERAND_RVKRNUM";
+ let OperandNamespace = "RISCVOp";
----------------
How does this work? The switch statement in RISCVInstrInfo.cpp (at e16d59973ffe, which is the ancestor of this commit) contains
> case RISCVOp::OPERAND_RVKRNUM:
> Ok = Imm >= 0 && Imm <= 10;
> break
So, how can we use the same enum for the various ranges ([0,7], ]1,10], [2,14], as well as [0, 10] from Zvk)?
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:107
+ defm VANDN_V : VALU_IV_V_X_I<"vandn", 0b000001>;
+ def VBREV8_V : VALUVs2<0b010010, 0b01000, OPIVV, "vbrev8.v">;
+ defm VCLMUL_V : VALU_IV_V_X_VCLMUL<"vclmul", 0b001100>;
----------------
Note that the spec had an inconsistency between OPIVV and OPMVV between different parts of the spec. The instruction description has been update to be list OPMVV for vbrev8 and vrev8.
<https://github.com/riscv/riscv-crypto/commit/71987c075aced13ad35d78122bbf69eecc3b4062>
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:141
+ def VSM4K_VI : PALUVINoVm<0b100001, "vsm4k.vi", rnum_0_7>;
+ def VSM4R_VV : PALUVs2NoVm<0b101000, 0b10000, OPMVV, "vsm4r.vv">;
+} // Predicates = [HasStdExtZvksed]
----------------
We still need to add "vsm4r.vs".
================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:46
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-zvknhb %s -o - | FileCheck --check-prefix=RV32ZVKNHB %s
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-zvkb %s -o - | FileCheck --check-prefix=RV32ZVKB %s
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-zvkg %s -o - | FileCheck --check-prefix=RV32ZVKG %s
----------------
Minor: let's use the alphabetical ordering (b/g/nha/nhb/ns/sed/sh), here and below.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvkb.s:89
+
+vror.vi v10, v9, 7, v0.t
+# CHECK-INST: vror.vi v10, v9, 7, v0.t
----------------
Minor: consider using a shift/rotate constant >= 32 here, to exercise the upper bit logic.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvknhb.s:1
+# RUN: llvm-mc -triple=riscv64 -show-encoding --mattr=+experimental-zvknhb %s \
+# RUN: | FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
----------------
Consider having a single "zvknh.s" file that tests both zvknha/zvknhb as the encodings are identical. This requires separate RUN lines (i.e., the concatenation of the RUN lines in both files), but the instructions and CHECK lines do not have to be duplicated.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvkns.s:65
+
+vaeskf2.vi v10, v9, 0
+# CHECK-INST: vaeskf2.vi v10, v9, 0
----------------
0 is not a valid round number for vaeskf2, this should be rejected.
Note: I asked Ken Dockser to update the pseudo-code to also test for values < 2, and also to consider removing the "rnd[3:0]" since we should be validating the whole 5 bits.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138807/new/
https://reviews.llvm.org/D138807
More information about the cfe-commits
mailing list