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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think I'm fine with this. Anyone else?



================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:27
+#define HANDLE_VP_TO_OC(VPID,OC)
+#endif
+
----------------
simoll wrote:
> jdoerfert wrote:
> > simoll wrote:
> > > jdoerfert wrote:
> > > > To be honest, I would have made the Opcode for vp intrinsics of existing functions `VP_EXISTING` (or similar) and used a single macro definition here, assuming all vp intrinsics so far have a unique corresponding scalar version.
> > > > These are two unrelated points you can consider (I won't force either).
> > > > 
> > > > (If you device to do both changes, you can even get away by only defining the existing opcode in the intrinsic "macro definition").
> > > > assuming all vp intrinsics so far have a unique corresponding scalar version.
> > > They don't. That's why it's separate. There will be more cases for constrained fp, reduction and memory ops.
> > > 
> > I see. Though, even if later intriniscs do not correspond to scalar instructions, the changes would still be possible. I admit, at this point it is a question if you like:
> > ```
> > REGISTER_VP_INTRINSIC(vp_add, 2, 3)
> > HANDLE_VP_TO_OC(vp_add, Add)
> > REGISTER_VP_INTRINSIC(vp_red, 2, 3)
> > ```
> > better or worse than
> > ```
> > REGISTER_VP_FROM_SCALAR_INTRINSIC(Add, 2, 3)
> > REGISTER_VP_INTRINSIC(Red, 2, 3)
> > ```
> > with the appropriate definition of the above:
> > ```
> > #define REGISTER_VP_FROM_SCALAR_INTRINSIC(OC, MP, VP) \
> >   REGISTER_VP_INTRINSIC(OC, MP, VP) \
> >   HANDLE_VP_TO_OC(vp_##OC, OC)
> > ```
> I prefer the explicit form
> 
>     REGISTER_VP_INTRINSIC(vp_add, 2, 3)
>     HANDLE_VP_TO_OC(vp_add, Add)
> 
> - The explicit form gives you much nicer diffs.
> - You don't have to read a meta macro to understand what is going on.
> - This file will undergo changes with the next patches. I wouldn't spend too much time compacting it before hasn't "settled down". 
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891