[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Eric Gouriou via Phabricator via cfe-commits]

ego added a comment.

See my comment below where I can trigger an MC-layer assertion when using 
"vaeskf1 v0, ...".




Comment at: llvm/test/MC/RISCV/rvv/rv64zvkns.s:1-9
+# RUN: llvm-mc -triple=riscv64 -show-encoding --mattr=+zve32x 
--mattr=+experimental-zvkns %s \
+# RUN:| FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
+# RUN: not llvm-mc -triple=riscv64 -show-encoding %s 2>&1 \
+# RUN:| FileCheck %s --check-prefix=CHECK-ERROR
+# RUN: llvm-mc -triple=riscv64 -filetype=obj --mattr=+zve32x 
--mattr=+experimental-zvkns %s \
+# RUN:| llvm-objdump -d --mattr=+zve32x --mattr=+experimental-zvkns  - 
\
+# RUN:| FileCheck %s --check-prefix=CHECK-INST

See Kito's comment. For most of those tests, and to avoid duplicating the logic 
between RV32 and RV64 files, we could drop the "rv64" prefix, cut/paste the RUN 
section to duplicate it and replace "riscv64" with "riscv32" in one of the 
copies.



Comment at: llvm/test/MC/RISCV/rvv/rv64zvkns.s:59
+
+vaeskf1.vi v10, v9, 1
+# CHECK-INST: vaeskf1.vi v10, v9, 1

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.dylib0x0001023015e8 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
1  libLLVMSupport.dylib0x000102300840 
llvm::sys::RunSignalHandlers() + 112
2  libLLVMSupport.dylib0x000102301c28 SignalHandler(int) + 304
3  libsystem_platform.dylib0x0001914f82a4 _sigtramp + 56
4  libsystem_pthread.dylib 0x0001914c9cec pthread_kill + 288
5  libsystem_c.dylib   0x0001914032c8 abort + 180
6  libsystem_c.dylib   0x000191402620 err + 0
7  libLLVMRISCVAsmParser.dylib 0x000100666208 (anonymous 
namespace)::RISCVAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned int&, 
llvm::SmallVectorImpl>>&, llvm::MCStreamer&, 
unsigned long long&, bool) (.cold.42) + 0
8  libLLVMRISCVAsmParser.dylib 0x000100655084 (anonymous 
namespace)::RISCVAsmParser::MatchAndEmitInstruction(llvm:

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Eric Gouriou via Phabricator via cfe-commits]

ego added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:135
+  defm VANDN_V : VALU_IV_V_X_I<"vandn", 0b01>;
+  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   
...
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   

```

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, ImmLeaf;
-  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   
+# CHECK-UNKNOWN: 57 25 94 48   
 
 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   
+# CHECK-UNKNOWN: 57 a5 94 48   
 
 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

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Eric Gouriou via Phabricator via cfe-commits]

ego added a comment.

FYI, more issues with v0 not being accepted as an operand with unmasked 
instructions.




Comment at: llvm/test/MC/RISCV/rvv/rv64zvkns.s:59
+
+vaeskf1.vi v10, v9, 1
+# CHECK-INST: vaeskf1.vi v10, v9, 1

ego wrote:
> 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.dylib0x0001023015e8 
> > > llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
> > > 1  libLLVMSupport.dylib0x000102300840 
> > > llvm::sys::RunSignalHandlers() + 112
> > > 2  libLLVMSupport.dylib0x000102301c28 SignalHandler(int) + 304
> > > 3  libsystem_platform.dylib0x0001914f82a4 _sigtramp + 56
> > > 4  libsystem_pthread.dylib 0x0001914c9cec pthread_kill + 288
> > > 5  libsystem_c.dylib   0x0001914032c8 abort + 180
> > > 6  libsystem_c.dylib   0x000191402620 err + 0
> > > 7  libLLVMRISCVAsmParser.dylib 0x000100666208 (anonymous 
> > > namespace)::RISCVAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned 
> > > int&, 
> > > llvm::SmallVectorImpl > > std::__1::default_delete>>&, llvm::MCStreamer&, 
> > > unsigned long long&, bool) (.cold.42) + 0
> > > 8  libLLVMRISCVAsmParser.dylib 0x000100655084 (anonymous 
> > > namespace)::RISCVAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned 
> > > int&, 
> > > llvm::SmallVectorImpl > > std::__1::default_delete>>&, llvm::MCStreamer&, 
> > > unsigned long long&, bool) + 7268
> > > 9  libLLVMMCParser.dylib   0x000100c36c08 (anonymous 
> > > namespace)::AsmParser::parseAndMatchAndEmitTargetInstruction

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Eric Gouriou via Phabricator via cfe-commits]

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 
`_.
 Note that current vector crypto extension version can be found in: 
.

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 funct6, string opcodestr, Operand optype>
+: VALUVINoVm {

This deserves a comment about intended usage.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:85
+
+def rnum_0_7 : Operand, ImmLeaf {

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 : AsmOperandClass {
  let Name = "Rnum" # min # "_" # max # "Operand";
  let RenderMethod = "addImmOperands";
  let DiagnosticType = "InvalidRnum" # min # "_" # max # "Operand";
}

// Oerand class representing cryptographic Round Number operands
// with d

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Eric Gouriou via Phabricator via cfe-commits]

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 {
+  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", 0b01>;
+  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.





Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:141
+  def VSM4K_VI : PALUVINoVm<0b11, "vsm4k.vi", rnum_0_7>;
+  def VSM4R_VV : PALUVs2NoVm<0b101000, 0b1, 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

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Eric Gouriou via Phabricator via cfe-commits]

ego added inline comments.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:827
+{{"zvkg"}, {ImpliedExtsZve32x}},
+{{"zvknha"}, {ImpliedExtsZve32x}},
+{{"zvknhb"}, {ImpliedExtsZve64x}},

ego wrote:
> 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.
> 
I had a chat with Ken Dockser on the topic. Ken's expectation is that one would 
have to explicitly declare a vector extension (e.g., 'v', or one of the 
"zve") for the Zvk* extensions to become usable. This matches my 
previous understanding, probably derived from earlier conversations with Ken.

If this is the intent of the code, the current logic in this file appears to be 
appropriate. However the attributes.ll test would then be in error, as it 
currently only specifies ```llc -mtriple=riscv32 -mattr=+experimental-zvknha 
...```.

As always I might be misinterpreting the intent of this patch. Don't hesitate 
to correct me :-) .


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