This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Zvk (vector crypto) specification update to 0.5.1 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg)
ClosedPublic

Authored by ego on Apr 16 2023, 4:17 PM.

Details

Summary

RISCV: Zvk update to 0.5.1 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg)

Update the Zvk support from 0.3.x to 0.5.1, tracking the extension as documented in https://github.com/riscv/riscv-crypto/releases/download/v20230407/riscv-crypto-spec-vector.pdf.

  • Zvkb is split into Zvbb and Zvbc
  • Zvbc (vector carryless multiply) requires 64 bit elements (Zve64x)
  • Use the extension descriptions from the specification for Zvbb/Zvbc
  • Zvkt is introduced (no instructions, but adds an attribute and macro)
  • Zvkn and Zvks both imply Zvkt
  • Zvkng and Zvksg are introduced, adding Zvkg (GMAC) to Zvkn and Zvks
  • In Zvbb, add vrev.v, vclz.v, vctz.v, vcpop.v, vwsll.{vv,vx,vi}

Diff Detail

Event Timeline

ego created this revision.Apr 16 2023, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 4:17 PM
ego requested review of this revision.Apr 16 2023, 4:17 PM
asb added a comment.Apr 18 2023, 8:37 AM

I started to review this - one thing I wanted to note quickly as it may impact other reviewers is that this update is written against something newer than the linked 0.4.6 release. I was confused about not being able to find zvksg and zvkng in the linked PDF, but see they were added a couple of weeks ago to the spec (i.e. after the 0.4.6 pdf) https://github.com/riscv/riscv-crypto/commit/6acf08361ed48eb85c5a562b54b99698d466f5f3

Looking at the commits since 0.4.6 https://github.com/riscv/riscv-crypto/compare/v20230322...master it seems zvksg/zvkng is the only change that's going to impact MC layer support and the version was bumped to 0.5.2, so perhaps 0.5 is a more accurate version number?

llvm/lib/Support/RISCVISAInfo.cpp
151

"zvkg" is duplicated

llvm/test/MC/RISCV/rvv/zvbb.s
107

Nit: I don't think you want a blank line here (only spotted as git apply complains).

asb added inline comments.Apr 18 2023, 8:53 AM
llvm/lib/Support/RISCVISAInfo.cpp
908

I'm not sure it makes sense to list composite extensions like zvkn and zvks and zvksg here, as the dependencies are going to be checked on the sub-extensions they imply anyway.

llvm/lib/Target/RISCV/RISCVFeatures.td
520

Zvkt => Zvkg

549

Zvkt => Zvkg

asb added a comment.Apr 18 2023, 8:54 AM

I think you're missing the llvm/test/CodeGen/RISCV/attributes.ll test changes too?

ego updated this revision to Diff 514695.Apr 18 2023, 11:20 AM
ego edited the summary of this revision. (Show Details)

Updated to address Alex's review. Also:

  • added a test for vgmul in rvv/zvkg.s,
  • in addition to fixing attributes.ll, added missing extension tests there,
  • added the missing feature for Zvkt, also caught with attributes.ll,
  • did actually include the clang-format fixes, instead of forgetting to add then to the amended commit.

I based the update on the latest specification PDF published, 0.5.1. I believe the later updates do not have impact on the support in LLVM.

Many thanks for catching so many real issues. As usual something weird happened since the patch was perfect before upload :-).

ego marked 5 inline comments as done.Apr 18 2023, 11:22 AM
ego added inline comments.
llvm/lib/Support/RISCVISAInfo.cpp
908

Thanks. I removed the combo extensions from the list.

ego edited the summary of this revision. (Show Details)Apr 18 2023, 11:24 AM
ego retitled this revision from RISC-V Zvk (vector crypto) specification update to 0.4 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg) to RISC-V Zvk (vector crypto) specification update to 0.5.1 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg).
ego added a reviewer: asb.
ego marked an inline comment as done.Apr 18 2023, 11:27 AM
ego added inline comments.
llvm/docs/RISCVUsage.rst
206

Ah, I forgot to update this version. Will upload another diff with this fixed.

ego updated this revision to Diff 514700.Apr 18 2023, 11:30 AM

Updated the version and PDF link in RISCVUsage.rst .

4vtomat added inline comments.Apr 18 2023, 6:22 PM
llvm/lib/Support/RISCVISAInfo.cpp
952

Maybe this should be in alphabetical order?

llvm/lib/Target/RISCV/RISCVFeatures.td
500

I guess this removal is NFC and not related to this patch~

503

I guess this removal is NFC and not related to this patch~

541

I guess this removal is NFC and not related to this patch~

553

I guess this removal is NFC and not related to this patch~

556

I guess this removal is NFC and not related to this patch~

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
131–145

This also need indentation.

136–137

Alphabetical order, thanks!
and remove blank line!

4vtomat added inline comments.Apr 18 2023, 6:29 PM
llvm/docs/RISCVUsage.rst
205–206

Maybe 'zvksed' is before 'zvksg'?

asb accepted this revision.Apr 19 2023, 9:24 AM

Many thanks for catching so many real issues. As usual something weird happened since the patch was perfect before upload :-).

That happens to me a lot too ;) All pretty minor stuff anyway.

All my previous comments were addressed and I stepped through the Zvbb and Zvbc definitions and the instruction+encoding definitions and couldn't spot any issues.

So LGTM - thanks!

You'll need to rebase before landing as some minor conflicting changes landed. Just a heads up that I tweaked what we do for external links in the experimental extension section https://reviews.llvm.org/rG0386546b088a (basically - using __ rather than _ for the links.

I don't really have a strong view about alphabetical vs other orderings, as long as it's consistent. For extension names, they seem to be listed alphabetically in the spec so that likely makes sense. There's not really a clear "right" ordering for instruction definitions - I used the order in the encoding table in the RISC-V spec for the base scalar instructions, but the quickref table in the vector crypto spec is nowhere near as easily readable so less useful for cross-checking. I'm neutral on that.

This revision is now accepted and ready to land.Apr 19 2023, 9:24 AM
ego updated this revision to Diff 515155.Apr 19 2023, 6:04 PM
ego marked 5 inline comments as done.
ego edited the summary of this revision. (Show Details)

I addressed Brandon's comments as best as I can:

  • this version removes the changes that dropped the '.' in the descriptions of the extensions (will propose in a follow-up NFC change). The result is slightly inconsistent as I did not use the '.' in the descriptions of Zvbb/Zvbc, we can figure what option to pick in follow-up.
  • the instruction definitions are sorted, hopefully as was suggested,
  • I fixed the ordering in the usage doc to be alphabetical,
  • I removed unintended trailing whitespace in a whitespace line in zvkg.s .

I did *not* rebase as I wanted to avoid bringing unrelated changes. Based on Alex description (and https://reviews.llvm.org/rG0386546b088a), this should be very simple.

I do not have commit access. I propose that, if everything looks good, I'll upload a rebased version and let someone (Alex?) commit on my behalf?
Let me know if there is a better way.

llvm/lib/Support/RISCVISAInfo.cpp
952

Sure, I'll send a follow-up to sort this series of constants and do the other proposed NFC changes.

llvm/lib/Target/RISCV/RISCVFeatures.td
500

Indeed, I should not have done this change in this commit.
I'll propose those changes in a separate PR/review/commit to decouple them from the Zvk update.

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
131–145

I do not understand this suggestion as I do not see missing indentation.
Are you suggesting to align the ':' ? In the comment above I argue against such alignment.
Please let me know.

136–137

It is (was) sorted, the order was due to 'def' vs. 'defm'.

Would you rather have it sorted by identifier (VANDN / VBREV8 / VBREV / VC* / etc.)? That's certainly doable but requires slightly more involved sorting than pure line sorting in an editor.

I feel that there are multiple orders that make sense, and each of them make the most sense in different circumstances:

  • by encoding (0b01000, ...)
  • by identifier (VANDN, ...), or by mnemonic (usually the same order, but doesn't have to be)
  • by "logical grouping" (e.g., VANDN, VCLZ/VCTZ, VCPOP, VBREV8/VBREV/VREV, VROL/VROR, VWSLL), but those are quite subjective
  • by sorting the lines, which will group "def" and "defm" separately.

For me the line sorting was the simplest, but it's easy enough to sort by the identifier( ctrl-u 2 M-x sort-fields in Emacs). I realize now that this is what your patch did for other extensions, so I'm happy to give it a try.

I feel that if we sort by identifier, it make sense to align the identifier, inserting a space to align "def" and "defm" identifiers. I checked the style guide and did not find an explicit recommendation against that additional vertical alignment. We don't want to align the ':' as it would force re-alignment if we introduce a larger identifier, forcing NFC changes in order to maintain that vertical alignment.

Let me know what you think.

asb added a comment.Apr 19 2023, 11:18 PM

I do not have commit access. I propose that, if everything looks good, I'll upload a rebased version and let someone (Alex?) commit on my behalf?
Let me know if there is a better way.

Yep, that's the usual way. Just indicate when it's ready for commit from your perspective and confirm the name + email address and one of us can commit. Whoever is around at the time and has been following this patch really - I believe we're in a different timezone but I'll happily commit if no-one beats me to it when you indicate it's ready.

See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access for details on obtaining commit access yourself - I'd suggest you email Chris after another landing another couple patches or so.

Additional changes LGTM from my perspective.

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
136–137

We often do add additioanl space before the : even though it has the disadvantage you mention of risking NFC changes if new identifiers are added. It has the advantage that when scanning definitions inheriting from the same class, the differences in class parameters are easier to spot as they're aligned (e.g. the changing bit pattern). You'll see examples of this e.g. in RISCVInstrInfoF.td

let SchedRW = [WriteFMA32, ReadFMA32, ReadFMA32, ReadFMA32] in {
defm FMADD_S  : FPFMA_rrr_frm_m<OPC_MADD,  0b00, "fmadd.s",  FINX>;
defm FMSUB_S  : FPFMA_rrr_frm_m<OPC_MSUB,  0b00, "fmsub.s",  FINX>;
defm FNMSUB_S : FPFMA_rrr_frm_m<OPC_NMSUB, 0b00, "fnmsub.s", FINX>;
defm FNMADD_S : FPFMA_rrr_frm_m<OPC_NMADD, 0b00, "fnmadd.s", FINX>;
}

But I don't think we're fully consistent with alignment style within lib/Target/RISCV so this isn't something I'd split hairs over.

4vtomat accepted this revision.Apr 19 2023, 11:34 PM

LGTM

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
131–145

You can skip this~

136–137

I think any alignment style is fine if it is consistent in a single file lol~
Thank you for your change!

ego updated this revision to Diff 515348.Apr 20 2023, 8:51 AM

Here is the rebased version.

Changes:

  • Rebased, with merges required in RISCVUsage.rst and attributes.ll
  • In RISCVUsage.rst, use "0.5" instead of "0.5.1" in relation to the Zvk extensions, since the ISA string needs to be specified as "zvbb0p5"
  • Vertically aligned the ':' in instruction definitions (def[m] MYINSTR_V : ....)
ego added a comment.EditedApr 20 2023, 8:57 AM

Thanks Alex, thanks Brandon for your reviews! The feedback was much appreciated.

I do not have commit access. I propose that, if everything looks good, I'll upload a rebased version and let someone (Alex?) commit on my behalf?
Let me know if there is a better way.

Yep, that's the usual way. Just indicate when it's ready for commit from your perspective and confirm the name + email address and one of us can commit. Whoever is around at the time and has been following this patch really - I believe we're in a different timezone but I'll happily commit if no-one beats me to it when you indicate it's ready.

Sounds good to me. The commit is ready from my point of view. Please use Eric Gouriou <ego@rivosinc.com>.

See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access for details on obtaining commit access yourself - I'd suggest you email Chris after another landing another couple patches or so.

Sounds good, will do.

We often do add additioanl space before the : even though it has the disadvantage you mention of risking NFC changes if new identifiers are added. It has the advantage that when scanning definitions inheriting from the same class, the differences in class parameters are easier to spot as they're aligned (e.g. the changing bit pattern).

Thanks for the clarification. I went with the additional alignment. I used to overdo vertical alignment but it was beaten out of me by the Google C++ style guide and automatic formatters.

ego updated this revision to Diff 515356.Apr 20 2023, 9:32 AM

New patch, to fix attributes.ll test failure.

I had messed up the conflict resolution. Zfa is now zfa0p2, needs to be listed as such.

craig.topper retitled this revision from RISC-V Zvk (vector crypto) specification update to 0.5.1 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg) to [RISCV] Zvk (vector crypto) specification update to 0.5.1 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg).Apr 20 2023, 9:38 AM
This revision was landed with ongoing or failed builds.Apr 20 2023, 10:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 10:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
asb added a comment.Apr 20 2023, 10:43 AM

Thanks for the clarification. I went with the additional alignment. I used to overdo vertical alignment but it was beaten out of me by the Google C++ style guide and automatic formatters.

Yes, once there's a decent .td auto-formatting solution I'll probably miss some of the niceties of manually aligned instruction definitions, but it will also be nice to spend less review time going back and forth on whitespace :)

Thanks for the rapid iteration on this - I've now landed the patch.

llvm/test/MC/RISCV/rvv/zvbc.s