[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: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.
+
----------------
simoll wrote:
> 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.
I understand that certain instructions take an explicit vector length parameter, but that doesn't really explain why an arch-independent intrinsic needs to expose this. Is there any reason that modelling this using something like `llvm.vp.enable.lanes` won't work?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:232
+    /// scalable.
+    Optional<int32_t> getStaticVectorLength() const;
+
----------------
simoll wrote:
> 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.
Awesome, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891