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

Reply via email to