This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Update Zvk shorthand extension to 1.0.0-rc1
ClosedPublic

Authored by Jim on Jul 23 2023, 11:33 PM.

Details

Summary
  1. Zvbc is not part of Zvkn.
  2. Zvbc is not part of Zvks.
  3. Add extensions implied into shorthand extension.

Also arrange its definition order as more closely its order in document.

Please refer to https://reviews.llvm.org/D152117 and https://reviews.llvm.org/D153836.

Document: https://github.com/riscv/riscv-crypto/releases/download/v20230620/riscv-crypto-spec-vector.pdf

Diff Detail

Unit TestsFailed

Event Timeline

Jim created this revision.Jul 23 2023, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2023, 11:33 PM
Jim requested review of this revision.Jul 23 2023, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2023, 11:33 PM
ego added a comment.Jul 24 2023, 1:08 AM

Thanks for fixing the expansion lists. I had missed them in the late Zvknc/Zvkng/... change of semantics.

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

As we are breaking the strict alphabetic ordering between Zvk sub-extensions, please add a marker to indicate the logic in the ordering.

For example:
// Zvk short-hand extensions

604

A few questions to improve my understanding. I am confused about the interactions between RISCVFeatures.td and RISCVISAInfo.cpp.

Prior to this commit, we did not list the implied extensions as part of the shorthand extensions. An expansion occurred during ISA string processing in llvm/lib/Support/RISCVISAInfo.cpp. Was this expansion missing in some cases, or are we now duplicating the expansion logic?

In RISCVISAInfo.cpp, there is also logic (see CombineIntoExts) to reveal the existence of those shorthand extensions when every component sub-extension has been declared. Is similar logic in place for those FeatureStdExt...?

Jim added inline comments.Jul 24 2023, 1:24 AM
llvm/lib/Target/RISCV/RISCVFeatures.td
604

I am also not sure should we add the impiled extension as part of the shorthand extension in RISCVFeatures.td.
That I saw FeatureStdExtZkn and FeatureStdExtZks has listed the impiled extensions.

ego added a comment.Jul 24 2023, 1:58 AM

LGTM as it seems to be a strict improvement (fixes incorrectly listed sub-extensions, encodes more information).

"git blame" points to Craig as a major contributor to this file, I'll defer to him on whether listing those expansions in two locations is the right thing to do.

RISCVFeatures.td does feature implication for llc -mattr= which does not use RISCVISAInfo.cpp. It could also be used by frontends other than clang that might not use RISCVISAInfo.cpp either. Those frontends might just expose something like llc's -mattr interface instead of having target specific logic.

There is no equivalent of CombineIntoExts for RISCVFeatures.td. The only use for CombineIntoExts right now is so the frontend will set the preprocessor define for the shorthand extension.

The duplication is unfortunate. Other targets also have some similar duplication.

craig.topper accepted this revision.Jul 24 2023, 7:17 AM

LGTM, other than Eric's request for a comment in the .td file.

This revision is now accepted and ready to land.Jul 24 2023, 7:17 AM
Jim updated this revision to Diff 543787.Jul 24 2023, 7:16 PM

Address comment.

Jim marked 2 inline comments as done.Jul 24 2023, 7:17 PM
This revision was landed with ongoing or failed builds.Jul 26 2023, 12:44 AM
This revision was automatically updated to reflect the committed changes.