jacquesguan marked 8 inline comments as done. jacquesguan added a comment. https://reviews.llvm.org/D151414 this is the backend part.
================ Comment at: clang/include/clang/Basic/riscv_vector.td:1856 def vfwcvt_f_x_v : RVVConvBuiltin<"Fw", "Fwv", "csi", "vfwcvt_f">; - def vfwcvt_f_f_v : RVVConvBuiltin<"w", "wv", "xf", "vfwcvt_f">; + let RequiredFeatures = ["ZvfhminOrZvfh"] in + def vfwcvt_f_f_v : RVVConvBuiltin<"w", "wv", "xf", "vfwcvt_f">; ---------------- michaelmaitland wrote: > michaelmaitland wrote: > > In general, I believe that `vfwcvt_f_f_v` and `vfncvt_f_f_w` do not require > > Zvfhmin or Zvfh. The only time that these intrinsics require Zvfhmin or > > Zvfh is when the operands to these intrinsics have EEW=16. > The semantics for `RequiredFeatures` is `Features required to enable for this > builtin.` Since not all types in the range require the ZvfhminOrZvfh feature, > it may make sense to do some refactoring: > > I think two possible solutions are: > 1. to split def of `vfwcvt_f_f_v` and `vfncvt_f_f_w ` by type_range and the > type range `x` uses the RequiredFeatures > 2. Use different required features for different type ranges (i.e. > RequiredFeatures is a list of lists where the outer list is for each type in > the range, and the inner list is the RequiredFeature for that type.) I split it into 2 definations. ================ Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin-error.c:15 + + + ---------------- michaelmaitland wrote: > Do we need a test that checks calls to `__riscv_vfwcvt_f` and > `__riscv_vfncvt_f` using type `vfloat16m1_t` without zvfh nor zvfhmin lead to > the expected error asking for `zvfh or zvfhmin`? fp16 vector type check is in `Sema/riscv-vector-float16-check.c`. ================ Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1018 + if (VT.getVectorElementType() == MVT::f16 && + !Subtarget.hasVInstructionsF16()) { ---------------- craig.topper wrote: > This needs to be below `By default everything must be expanded.` Fixed in https://reviews.llvm.org/D151414. ================ Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2096 case MVT::f16: - return Subtarget.hasVInstructionsF16(); + return Subtarget.hasVInstructionsF16Mininal(); case MVT::f32: ---------------- craig.topper wrote: > Is this correct? This function is called by > `RISCVTargetTransformInfo::isLegalToVectorizeReduction`, > `isLegalMaskedLoadStore`, `isLegalMaskedGatherScatter`. Fixed in https://reviews.llvm.org/D151414. ================ Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:162 + bool hasVInstructionsF16Mininal() const { + return HasStdExtZvfhmin || HasStdExtZvfh; + } ---------------- craig.topper wrote: > Doesn't HasStdExtZvfh already imply HasStdExtZvfhmin? The v spec doesn't metion this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150253/new/ https://reviews.llvm.org/D150253 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits