[PATCH] D112408: [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec

[Zakk Chen via Phabricator via llvm-commits]

khchen added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:181
   }
 
+  if (MinVLen) {
----------------
please add a note in commit or comment here for those macros are proposed in the PR https://github.com/riscv-non-isa/riscv-c-api-doc/pull/21


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:184
     Builder.defineMacro("__riscv_v_min_vlen", Twine(MinVLen));
+    Builder.defineMacro("__riscv_v_max_eew", Twine(MaxEew));
+    Builder.defineMacro("__riscv_v_max_eew_fp", Twine(MaxEewFp));
----------------
craig.topper wrote:
> Would't ELEN be the correct term here? Not EEW.
https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#182-zve-vector-extensions-for-embedded-processors shows zve* extensions have `Supported EEW`, I guess it's why the term is `EEW`.




================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:735
   const StringMap<std::vector<StringRef>> Implications = {
-    {"v", {"zvlsseg", "zvl128b"}},
+    {"v", {"zvlsseg", "zvl128b", "zve64d"}},
+
----------------
please update implication rule based on https://github.com/riscv/riscv-v-spec/issues/723#issuecomment-949542444
1. The Zve32f and Zve64f extensions depend upon the F extension
2. The Zve64d extension depends upon the D extension
3. The V extension depends upon the F and D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112408