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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
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)
+
----------------
jdoerfert wrote:
> sdesmalen wrote:
> > I expect this to also work for scalable vectors, so maybe add a case here for `<vscale x W x T>` as well?
> If that support is added later, I would add the wording later too.
@simoll is anything special needed to support scalable vectors in this context?


================
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.
+
----------------
jdoerfert wrote:
> simoll wrote:
> > sdesmalen wrote:
> > > 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.
> > Yes i considered that. However, as you suggested you can express this without loss with a  `select` and i'd like to keep the intrinsics simple.
> FWIW, I like the idea of simple intrinsics and explicit selection.
If we want to keep the vp intrinsics simple, shouldn't we reconsider the need for `%evl` as an explicit parameter?

That value can be folded into the predicate using a special intrinsic that enables the lanes upto `%evl`, e.g.

`<256 x i1> llvm.vp.enable.lanes(<256 x i1> %p, i32 %evl)`

If any operation needs `%evl` as an explicit parameter, it would be a matter of having a pattern that matches the enable.lanes intrinsic, similar to how merging/zeroing predication can be implemented by matching the selects.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:232
+    /// scalable.
+    Optional<int32_t> getStaticVectorLength() const;
+
----------------
Can this be renamed to `getVectorLength()` and have it return `ElementCount` instead? (at which point we can drop the Optional)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891