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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:186
+                       "with maximal 32 EEW and F extension)",
+                       [FeatureStdExtZve32x, FeatureStdExtF]>;
+def HasStdExtZve32f : Predicate<"SubTarget->hasStdExtZve32f()">;
----------------
Zve32f requires F or Zfinx. It can't imply F.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:206
+                       "with maximal 64 EEW, F and D extension)",
+                       [FeatureStdExtZve64f, FeatureStdExtD]>;
+def HasStdExtZve64d : Predicate<"SubTarget->hasStdExtZve64d()">;
----------------
Zve64d requires D or Zdinx. It can't imply D.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:209
+
+def FeatureStdExtV
+    : SubtargetFeature<"experimental-v", "HasStdExtV", "true",
----------------
V can imply F and D though I think.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:864
+
+let Predicates = [IsRV64, HasVInstructionsI64] in {
+// Vector Unit-Stride Instructions
----------------
Only the indexed load/stores require RV64 for EEW=64. Sorry I missed that when I caught it for the segment load/store.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:192
+  bool hasVInstructionsI64() const { return HasStdExtZve64x; }
+  bool hasVInstructionsF16() const { return HasStdExtZve32x && HasStdExtZfh; }
+  bool hasVInstructionsF32() const { return HasStdExtZve32f; }
----------------
Zve32f I think since Zfh requires F.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:193
+  bool hasVInstructionsF16() const { return HasStdExtZve32x && HasStdExtZfh; }
+  bool hasVInstructionsF32() const { return HasStdExtZve32f; }
+  bool hasVInstructionsF64() const { return HasStdExtZve64d; }
----------------
I think we need to check HasStdExtF with Zve32f with a FIXME to consider HasStdExtZfinx in the future.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:194
+  bool hasVInstructionsF32() const { return HasStdExtZve32f; }
+  bool hasVInstructionsF64() const { return HasStdExtZve64d; }
   // F16 and F64 both require F32.
----------------
I think we need to check HasStdExtD with Zve64d with a FIXME to consider HasStdExtZdinx in the future


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112408