[PATCH] D156084: [RISCV] Update Zvk shorthand extension to 1.0.0-rc1

Mon Jul 24 01:08:42 PDT 2023

ego added a comment.

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

================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:599
                        "'Zvkt' (Vector Data-Independent Execution Latency)">;

+def FeatureStdExtZvkn
----------------
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

================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:604
+                       "other extensions: Zvkned, Zvknhb, Zvbb and Zvkt.",
+                       [FeatureStdExtZvkned, FeatureStdExtZvknhb,
+                        FeatureStdExtZvbb, FeatureStdExtZvkt]>;
----------------
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...?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156084/new/

https://reviews.llvm.org/D156084