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

[Simon Moll via Phabricator via llvm-commits]

simoll 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)
+
----------------
sdesmalen wrote:
> 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?
> @simoll is anything special needed to support scalable vectors in this context?

The intrinsics support scalable vectors out of the box. I've added VP intrinsics with scalable types to the individual operations below.


================
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.
+
----------------
sdesmalen wrote:
> 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.
> 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.

The SX-Aurora and RiSC-V V extension natively support an explicit vector length parameter just as they support predicated vector. Mask and EVL should be treated the same: if there is a vector mask parameter there should also be an EVL parameter.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891