[PATCH] D134586: [VP][RISCV] Add vp.ceil and RISC-V support

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/docs/LangRef.rst:21230
+
+Predicated floating-point ceil value of a vector of floating-point values.
+
----------------
"ceil value" -> ceiling


================
Comment at: llvm/docs/LangRef.rst:21244
+
+The '``llvm.vp.ceil``' intrinsic performs floating-point ceil value
+(:ref:`ceil <int_ceil>`) of the first vector operand on each enabled lane.  The
----------------
"ceil value" -> ceiling


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:328
+    // FIXME: This should be guarded correctly by zvfh in the future
+    setOperationAction(ISD::VP_CEIL, MVT::f16, Promote);
+
----------------
VP_CEIL is only valid for vectors. MVT::f16 is a scalar.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:662
 
+      setOperationAction(ISD::VP_CEIL, VT, Custom);
+
----------------
Isn't this already done by line 701 after updating `FloatingPointVPOps`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:908
 
+        setOperationAction(ISD::VP_CEIL, VT, Custom);
+
----------------
Already done by line 919?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1984
+
+  if (!IsVP) {
+    auto [TrueMask, MaxVL] =
----------------
Swap the if and else so `IsVP` doesn't need to be inverted


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1985
+  if (!IsVP) {
+    auto [TrueMask, MaxVL] =
+        getDefaultVLOps(VT, ContainerVT, DL, DAG, Subtarget);
----------------
Use `std::tie(Mask, VL) = getDefaultVLOps(VT, ContainerVT, DL, DAG, Subtarget)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134586