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

[Simon Moll via Phabricator via llvm-commits]

simoll planned changes to this revision.
simoll marked 12 inline comments as done.
simoll added a comment.

Thanks for the comments so far :)



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1172
+
+}
+
----------------
jdoerfert wrote:
> simoll wrote:
> > jdoerfert wrote:
> > > I guess you can add readnone as an attribute, also nosync and nofree please.
> > The intrinsics are `readnone` already. `nosync` seems sensible.
> I asked @sstefan1 to add nosync and nofree to the td file and other places needed to use them. That should make your changes more concise.
> 
> ---
> 
> I would have argued we could even do the `#inline` trick here, or maybe some .td magic, to avoid this replication, but I will not force anything.
> I would have argued we could even do the `#inline` trick here, or maybe some .td magic, to avoid this replication, but I will not force anything.

I agree but refactoring `Intrinsics.td` is out of scope for this patch.


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



================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:244
+  return GetStaticVectorLengthOfType(VPMask->getType());
+}
+
----------------
jdoerfert wrote:
> Does it make sense to have this corner case in here or should we just make it an `Optional<int64_t>` and call it a day?
> 
> You can also remove the lambda if you move the code, except there is a reason to keep it.
Actually, the excessive vector length case should `return None` to imply that the static vector length could not be inferred.

Btw, later patches will reuse the lambda.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:306
+  default:
+    return Instruction::Call;
+
----------------
jdoerfert wrote:
> I doubt this is a sensible default. I doubt one should return anything for a non-vp intrinsic.
If there is no functional IR opcode this is still a call to an intrinsic function. It's natural to return `Call` in this case.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:353
+}
+
 Instruction::BinaryOps BinaryOpIntrinsic::getBinaryOp() const {
----------------
jdoerfert wrote:
> Nit: clang format your changes (or patches) (clang-format.py for vim has the formatdiff option)
This particular function is clang-formatted already.. The existing code in `IntrinsicInst.cpp` is not. I'll do a patch clang-formatting the **entire** file after this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891