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