[PATCH] D69891: [VP,Integer,#1] Vector-predicated integer intrinsics

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for your efforts to move this forward @simoll!



================
Comment at: llvm/docs/LangRef.rst:14666
+
+The vector mask parameter always has a vector of bit type, for example `<32 x i1>`.
+The explicit vector length parameter always has the type `i32`.
----------------
nit: bit -> i1


================
Comment at: llvm/docs/LangRef.rst:14670
+Results are only computed for enabled lanes.
+A lane is enabled if the mask at that position is true and, if effective, where the lane position is below the explicit vector length.
+
----------------
nit: 80 char limit (also for other places in this file)


================
Comment at: llvm/docs/LangRef.rst:14664
+
+      <W x T> llvm.vp.<opcode>.*(<W x T> %x, <W x T> %y, <W x i1> %mask, i32 %evl)
+
----------------
I expect this to also work for scalable vectors, so maybe add a case here for `<vscale x W x T>` as well?


================
Comment at: llvm/docs/LangRef.rst:14668
+The explicit vector length parameter always has the type `i32`.
+The explicit vector length is only effective if the MSB of its value is zero.
+Results are only computed for enabled lanes.
----------------
simoll wrote:
> SjoerdMeijer wrote:
> > Nit: instead of talking about the MSB being zero, is it simpler to say that it is effective if is a positive number (if that's what you mean)?
> Good point. I'll rewrite that part referring to the vlen as a signed integer.
Thanks, I wondered something similar. 

I had actually expected the `%evl` to only make sense in the context of scalable vectors?

Is it worth expressing what it means if `%evl` is larger than the size of the vector?


================
Comment at: llvm/docs/LangRef.rst:14702
+The '``llvm.vp.add``' intrinsic performs integer addition (:ref:`add <i_add>`) of the first and second vector operand on each enabled lane.
+The result on disabled lanes is undefined.
+
----------------
Have you considered adding an extra argument to specify the value of the false lanes, similar to how this is currently done for `llvm.masked.load`? 
By passing `undef`, it would have similar behaviour as the current definition, yet it would remove the need to add explicit `select` statements for handling merging- or zeroing predication if an instruction supports it. For SVE for example, most instructions have either merging or zeroing predication and the intrinsics expose the merging/zeroing/undef predication directly in the C/C++ intrinsic API. I think it would be really nice to have that capability represented in the vp intrinsics as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891