[PATCH] D135418: [VP][RISCV] Add vp.smax/smin/umax/umin intrinsics

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

eopXD added a comment.

Looks good to me except a nit and a question to the FIXME. I would prefer another person to also review the patch and approve it though.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:458
+        ISD::VP_ZERO_EXTEND, ISD::VP_TRUNCATE,
+        ISD::VP_SMIN, ISD::VP_SMAX, ISD::VP_UMIN, ISD::VP_UMAX };
 
----------------
This should trigger clang-foramt.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vmax-vp.ll:9
+
+define <8 x i7> @vmax_vv_v8i7(<8 x i7> %va, <8 x i7> %b, <8 x i1> %m, i32 zeroext %evl) {
+; CHECK-LABEL: vmax_vv_v8i7:
----------------
Nit: `%b` to `%vb`


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vmax-vp.ll:364
+
+; FIXME: The upper half is doing nothing.
+
----------------
Just to make sure, do you mean that LMUL should be m4 here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135418