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

[Simon Moll via Phabricator via llvm-commits]

simoll planned changes to this revision.
simoll marked 14 inline comments as done.
simoll added inline comments.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:1
+//===-- llvm/Instruction.def - File that describes Instructions -*- C++ -*-===//
+//
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > Nit: copy&past
> now too long and wrapped :(
> No need for `llvm/`, which is confusing as it lives in `llvm/IR`.
> Also "Instructions" might be a bit to broad? Idk, maybe: "vector intrinsic descriptions"
   //===-- IR/VPInstruction.def - Describes llvm.vp.* Intrinsics -*- C++ -*-===//



================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:27
+#define HANDLE_VP_TO_OC(VPID,OC)
+#endif
+
----------------
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". 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891