[PATCH] D122291: [VP] Add more cast VPintrinsic and docs.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck requested changes to this revision.
frasercrmck added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/docs/LangRef.rst:20258
+The '``llvm.vp.zext``' intrinsic takes a value to cast as its first operand.
+The return type is the type to cast the value to. Both types must be vector of
+:ref:`integer <t_integer>` type. The bit size of the value must be smaller than
----------------
`must be vectors`


================
Comment at: llvm/docs/LangRef.rst:20259
+The return type is the type to cast the value to. Both types must be vector of
+:ref:`integer <t_integer>` type. The bit size of the value must be smaller than
+the bit size of the return type. The second operand is the vector mask. The
----------------
in `trunc` we explicitly say it cannot be a no-op cast. Should we say that here too?


================
Comment at: llvm/docs/LangRef.rst:20282
+      %t = zext <4 x i16> %a to <4 x i32>
+      %also.r = select <4 x i1> %mask, <4 x i16> %t, <4 x i16> undef
+
----------------
these select ops should be `<4 x i32>`


================
Comment at: llvm/docs/LangRef.rst:20310
+The '``llvm.vp.sext``' intrinsic takes a value to cast as its first operand.
+The return type is the type to cast the value to. Both types must be vector of
+:ref:`integer <t_integer>` type. The bit size of the value must be smaller than
----------------
`vectors` again.


================
Comment at: llvm/docs/LangRef.rst:20373
+
+The '``llvm.vp.trunc``' intrinsic casts a ``value`` from a larger
+:ref:`floating-point <t_floating>` type to a smaller :ref:`floating-point
----------------
`trunc` -> `fptrunc`


================
Comment at: llvm/docs/LangRef.rst:20404
+
+      declare <16 x double>  @llvm.vp.fpext.v16f64.v16f64 (<16 x float> <op>, <16 x i1> <mask>, i32 <vector_length>)
+      declare <vscale x 4 x double>  @llvm.vp.fpext.nxv4f32.nxv4f64 (<vscale x 4 x float> <op>, <vscale x 4 x i1> <mask>, i32 <vector_length>)
----------------
`v16f64.v16f32`?


================
Comment at: llvm/docs/LangRef.rst:20405
+      declare <16 x double>  @llvm.vp.fpext.v16f64.v16f64 (<16 x float> <op>, <16 x i1> <mask>, i32 <vector_length>)
+      declare <vscale x 4 x double>  @llvm.vp.fpext.nxv4f32.nxv4f64 (<vscale x 4 x float> <op>, <vscale x 4 x i1> <mask>, i32 <vector_length>)
+
----------------
other way round - `nxv4f64.nxv4f32`?


================
Comment at: llvm/docs/LangRef.rst:20502
+      %t = fptoui <4 x float> %a to <4 x i32>
+      %also.r = select <4 x i1> %mask, <4 x float> %t, <4 x float> undef
+
----------------
select operands should be `<4 x i32>` (I just pushed cc67a8fcf148 to fix it in `vp.fptosi`.)


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:271
+
+#define HELPER_REGISTER_CAST_VP(OPSUFFIX, VPSD, IROPC)                         \
+  BEGIN_REGISTER_VP(vp_##OPSUFFIX, 1, 2, VPSD, -1)                             \
----------------
Can't we have `HELPER_REGISTER_INT_CAST_VP` or something? This just makes it harder to navigate the codebase imo.


================
Comment at: llvm/lib/IR/Verifier.cpp:5604
+
+    switch (VPCast->getIntrinsicID()) {
+    default:
----------------
I'd like to see some tests in `test/Verifier/invalid-vp-intrinsics.ll` for this


================
Comment at: llvm/lib/IR/Verifier.cpp:5607
+      llvm_unreachable("Unknown VP cast intrinsic");
+    case Intrinsic::vp_trunc: {
+      Assert(RetTy->isIntOrIntVectorTy() && ValTy->isIntOrIntVectorTy(),
----------------
You don't need `{}` in any of these switch cases


================
Comment at: llvm/test/Verifier/vp-intrinsics.ll:60
 
-define void @test_vp_int_fp_conversions(<8 x i32> %i0, <8 x float> %f0, <8 x i1> %mask, i32 %evl) {
-  %r0 = call <8 x float> @llvm.vp.sitofp.v8f32.v8i32(<8 x i32> %i0, <8 x i1> %mask, i32 %evl)
+define void @test_vp_int_fp_conversions(<8 x i32*> %p0, <8 x i32> %i0, <8 x i64> %i1, <8 x float> %f0, <8 x double> %f1, <8 x i1> %mask, i32 %evl) {
+  %r0 = call <8 x i32> @llvm.vp.fptoui.v8i32.v8f32(<8 x float> %f0, <8 x i1> %mask, i32 %evl)
----------------
Feels to me like the name `test_vp_int_fp_conversions` implies it's testing //only// int<->fp conversions. Maybe we should have `test_vp_int_conversions` for `sext`/`zext`/`trunc`/`ptrtoint`/`inttoptr` and `test_vp_fp_conversions` for `fpext`/`fptrunc`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122291