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

[Eric Gouriou via Phabricator via llvm-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<something>") 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