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

[Jianjian Guan via Phabricator via llvm-commits]

jacquesguan added inline comments.


================
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"> {
----------------
eopXD wrote:
> craig.topper wrote:
> > eopXD wrote:
> > > 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.
> > Note that the spec says Zfhmin(no v) not Zvfhmin. 
> My mistake. I suspect this is an oversight of the v-spec, just created an issue for this.
> 
> https://github.com/riscv/riscv-v-spec/issues/885
Thanks for comment. I think that v-spec is in accordance with the scalar spec to some degree. Since `zfh` doesn't imply `zfhmin`, I don't think they will let `zvfh` imply `zvfhmin`.


================
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);
----------------
eopXD wrote:
> 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
Yes, but this test is for zvfhmin. `vfncvt.c` contains other convert cases not enable for zvfhmin so we can't just add a zvfhmin check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150253