[PATCH] D150253: [RISCV] Add Zvfhmin extension for clang.

[Yueh-Ting (eop) Chen via Phabricator via cfe-commits]

eopXD added a comment.

Thank you for the patch. Few comments here.



================
Comment at: clang/include/clang/Basic/riscv_vector.td:2219
+  def vfwcvt_f_f_v : RVVConvBuiltin<"w", "wv", "f", "vfwcvt_f">;
+  let RequiredFeatures = ["ZvfhminOrZvfh"] in
+    def vfwcvt_f_f_v_fp16 : RVVConvBuiltin<"w", "wv", "x", "vfwcvt_f"> {
----------------
I think using `ZvfhminOrZvfh` is not accurate here. By the v-spec:

> When the Zvfhmin extension is implemented, the vfwcvt.f.f.v and vfncvt.f.f.w instructions become defined when SEW=16. 
> The Zvfh extension depends on the Zve32f and Zfhmin extensions.

I think making it `let RequiredFeatures = ["Zvfhmin"]` would be clearer.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:199
+  bool HasZvfh = TI.hasFeature("experimental-zvfh");
+  bool HasZvfhminOrZvfh = TI.hasFeature("experimental-zvfhmin") || HasZvfh;
 
----------------
I think `HasZvfhmin` is a more accurate naming.


================
Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin-error.c:20
+
+// CHECK-ZVFHMIN-ERR: no matching function for call to '__riscv_vfadd'
+
----------------
If `zvfhmin` is not specified, should the compiler emit semantic error when encountering `vfloat16*_t` types?


================
Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin.c:15
+//
+vfloat16m1_t test_vfncvt_f_f_w_f16m1(vfloat32m2_t src, size_t vl) {
+  return __riscv_vfncvt_f(src, vl);
----------------
This test case is already covered.

https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vfncvt.c#L356


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150253