[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 Jan 3 09:51:52 PST 2023
ego added inline comments.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:135
+ 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>;
----------------
This ought to be OPMVV.
https://github.com/riscv/riscv-crypto/blob/master/doc/vector/insns/vbrev8.adoc
Doing OPIVV->OPMVV sets bit 13, adding 0x20 to the second byte in the CHECK-ENCODING tests for vbrev8 and vrev8
```
vbrev8.v v10, v9, v0.t
# CHECK-INST: vbrev8.v v10, v9, v0.t
# CHECK-ENCODING: [0x57,0x25,0x94,0x48]
# CHECK-ERROR: instruction requires the following: 'Zvkb'
# CHECK-UNKNOWN: 57 25 94 48 <unknown>
...
vrev8.v v10, v9, v0.t
# CHECK-INST: vrev8.v v10, v9, v0.t
# CHECK-ENCODING: [0x57,0xa5,0x94,0x48]
# CHECK-ERROR: instruction requires the following: 'Zvkb'
# CHECK-UNKNOWN: 57 a5 94 48 <unknown>
```
diff:
```
GIT_PAGER= git diff
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
index b44bf7476808..3477c8d860ac 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
@@ -132,10 +132,10 @@ def rnum_2_14 : Operand<XLenVT>, ImmLeaf<XLenVT,
let Predicates = [HasStdExtZvkb] in {
defm VANDN_V : VALU_IV_V_X_I<"vandn", 0b000001>;
- def VBREV8_V : VALUVs2<0b010010, 0b01000, OPIVV, "vbrev8.v">;
+ def VBREV8_V : VALUVs2<0b010010, 0b01000, OPMVV, "vbrev8.v">;
defm VCLMUL_V : VALU_IV_V_X_VCLMUL<"vclmul", 0b001100>;
defm VCLMULH_V : VALU_IV_V_X_VCLMUL<"vclmulh", 0b001101>;
- def VREV8_V : VALUVs2<0b010010, 0b01001, OPIVV, "vrev8.v">;
+ def VREV8_V : VALUVs2<0b010010, 0b01001, OPMVV, "vrev8.v">;
defm VROL_V : VALU_IV_V_X<"vrol", 0b010101>;
defm VROR_V : VALU_IV_V_X_I_VROR<"vror", 0b010100>;
} // Predicates = [HasStdExtZvkb]
diff --git a/llvm/test/MC/RISCV/rvv/rv64zvkb.s b/llvm/test/MC/RISCV/rvv/rv64zvkb.s
index f4f7d9a038fc..d1600296e7e6 100644
--- a/llvm/test/MC/RISCV/rvv/rv64zvkb.s
+++ b/llvm/test/MC/RISCV/rvv/rv64zvkb.s
@@ -28,9 +28,9 @@ vandn.vi v10, v9, 7, v0.t
vbrev8.v v10, v9, v0.t
# CHECK-INST: vbrev8.v v10, v9, v0.t
-# CHECK-ENCODING: [0x57,0x05,0x94,0x48]
+# CHECK-ENCODING: [0x57,0x25,0x94,0x48]
# CHECK-ERROR: instruction requires the following: 'Zvkb'
-# CHECK-UNKNOWN: 57 05 94 48 <unknown>
+# CHECK-UNKNOWN: 57 25 94 48 <unknown>
vclmul.vv v10, v9, v8
# CHECK-INST: vclmul.vv v10, v9, v8
@@ -58,9 +58,9 @@ vclmulh.vx v10, v9, a0
vrev8.v v10, v9, v0.t
# CHECK-INST: vrev8.v v10, v9, v0.t
-# CHECK-ENCODING: [0x57,0x85,0x94,0x48]
+# CHECK-ENCODING: [0x57,0xa5,0x94,0x48]
# CHECK-ERROR: instruction requires the following: 'Zvkb'
-# CHECK-UNKNOWN: 57 85 94 48 <unknown>
+# CHECK-UNKNOWN: 57 a5 94 48 <unknown>
vrol.vv v10, v9, v8, v0.t
# CHECK-INST: vrol.vv v10, v9, v8, v0.t
```
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:138
+ defm VCLMULH_V : VALU_IV_V_X_VCLMUL<"vclmulh", 0b001101>;
+ def VREV8_V : VALUVs2<0b010010, 0b01001, OPIVV, "vrev8.v">;
+ defm VROL_V : VALU_IV_V_X<"vrol", 0b010101>;
----------------
This ought to be OPMVV.
https://github.com/riscv/riscv-crypto/blob/master/doc/vector/insns/vrev8.adoc
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvknh.s:19-20
+vsha2ms.vv v10, v9, v8
+# CHECK-INST-ZVKNHA: vsha2ms.vv v10, v9, v8
+# CHECK-INST-ZVKNHB: vsha2ms.vv v10, v9, v8
+# CHECK-ENCODING-ZVKNHA: [0x77,0x25,0x94,0xb6]
----------------
While I do understand why we want to run the tests with each of the two extensions declared, I don't understand the value of replicating the checks in the cases where there is no difference between A and B. I do not see any case where the expectations differ since the instructions exist in both cases, have the same encodings, and the error string covers both extensions.
If we want to communicate that both behave the same, a comment would convey this more clearly.
================
Comment at: llvm/test/MC/RISCV/rvv/rv64zvkns.s:59
+
+vaeskf1.vi v10, v9, 1
+# CHECK-INST: vaeskf1.vi v10, v9, 1
----------------
craig.topper wrote:
> ego wrote:
> > If I replaces "v10" with "v0", the test fails with an assertion failure. My own patch uses a slightly different class hierarchy but hits the same assertion.
> >
> >
> > ```
> > FAIL: LLVM :: MC/RISCV/rvv/rv64zvkns.s (3 of 8)
> > ******************** TEST 'LLVM :: MC/RISCV/rvv/rv64zvkns.s' FAILED ********************
> > Script:
> > --
> > : 'RUN: at line 1'; /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-mc -triple=riscv64 -show-encoding --mattr=+zve32x --mattr=+experimental-zvkns /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s | /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/FileCheck /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s --check-prefixes=CHECK-ENCODING,CHECK-INST
> > : 'RUN: at line 3'; not /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-mc -triple=riscv64 -show-encoding /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s 2>&1 | /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/FileCheck /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s --check-prefix=CHECK-ERROR
> > : 'RUN: at line 5'; /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-mc -triple=riscv64 -filetype=obj --mattr=+zve32x --mattr=+experimental-zvkns /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s | /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-objdump -d --mattr=+zve32x --mattr=+experimental-zvkns - | /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/FileCheck /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s --check-prefix=CHECK-INST
> > : 'RUN: at line 8'; /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-mc -triple=riscv64 -filetype=obj --mattr=+zve32x --mattr=+experimental-zvkns /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s | /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-objdump -d - | /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/FileCheck /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s --check-prefix=CHECK-UNKNOWN
> > --
> > Exit Code: 2
> >
> > Command Output (stderr):
> > --
> > Assertion failed: (isReg() && "This is not a register operand!"), function getReg, file MCInst.h, line 70.
> > PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
> > Stack dump:
> > 0. Program arguments: /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/llvm-mc -triple=riscv64 -show-encoding --mattr=+zve32x --mattr=+experimental-zvkns /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s
> > Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
> > 0 libLLVMSupport.dylib 0x00000001023015e8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
> > 1 libLLVMSupport.dylib 0x0000000102300840 llvm::sys::RunSignalHandlers() + 112
> > 2 libLLVMSupport.dylib 0x0000000102301c28 SignalHandler(int) + 304
> > 3 libsystem_platform.dylib 0x00000001914f82a4 _sigtramp + 56
> > 4 libsystem_pthread.dylib 0x00000001914c9cec pthread_kill + 288
> > 5 libsystem_c.dylib 0x00000001914032c8 abort + 180
> > 6 libsystem_c.dylib 0x0000000191402620 err + 0
> > 7 libLLVMRISCVAsmParser.dylib 0x0000000100666208 (anonymous namespace)::RISCVAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned int&, llvm::SmallVectorImpl<std::__1::unique_ptr<llvm::MCParsedAsmOperand, std::__1::default_delete<llvm::MCParsedAsmOperand>>>&, llvm::MCStreamer&, unsigned long long&, bool) (.cold.42) + 0
> > 8 libLLVMRISCVAsmParser.dylib 0x0000000100655084 (anonymous namespace)::RISCVAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned int&, llvm::SmallVectorImpl<std::__1::unique_ptr<llvm::MCParsedAsmOperand, std::__1::default_delete<llvm::MCParsedAsmOperand>>>&, llvm::MCStreamer&, unsigned long long&, bool) + 7268
> > 9 libLLVMMCParser.dylib 0x0000000100c36c08 (anonymous namespace)::AsmParser::parseAndMatchAndEmitTargetInstruction((anonymous namespace)::ParseStatementInfo&, llvm::StringRef, llvm::AsmToken, llvm::SMLoc) + 1356
> > 10 libLLVMMCParser.dylib 0x0000000100c2d1d0 (anonymous namespace)::AsmParser::parseStatement((anonymous namespace)::ParseStatementInfo&, llvm::MCAsmParserSemaCallback*) + 3492
> > 11 libLLVMMCParser.dylib 0x0000000100c27884 (anonymous namespace)::AsmParser::Run(bool, bool) + 500
> > 12 llvm-mc 0x000000010051ffc8 main + 6608
> > 13 dyld 0x000000019119fe50 start + 2544
> > FileCheck error: '<stdin>' is empty.
> > FileCheck command line: /Users/ego/root-dirs/scratch/projects/llvm-project/build/bin/FileCheck /Users/ego/root-dirs/scratch/projects/llvm-project/llvm/test/MC/RISCV/rvv/rv64zvkns.s --check-prefixes=CHECK-ENCODING,CHECK-INST
> >
> > --
> >
> > ```
> Looks like it ended up with `RVVConstraint = VMConstraint` due to the classes it inherited. This caused validateInstruction to look for a mask operand that doesn't exist. Need to force it to the NoConstraint.
Thanks Craig, that "NoConstraint" worked for me.
It would be good to check if the same issue exists with other instructions in their no-mask uses.
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