[PATCH] D124358: [LV][SLP] Mark fptosi_sat as vectorizable

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:320
 /// an overloaded type.
-bool hasVectorInstrinsicOverloadedScalarOpd(Intrinsic::ID ID,
-                                            unsigned ScalarOpdIdx);
+bool hasVectorInstrinsicOverloadedOpd(Intrinsic::ID ID, unsigned OpdIdx);
 
----------------
spatel wrote:
> Existing typo in the names here and above - "Instrinsic" -> "Intrinsic".
> On top of that, these function names don't read clearly to me, so if we're changing things anyway, maybe we can do better.
> "isVectorIntrinsicWithScalarOpAtArg"
> "isVectorIntrinsicWithDifferentTypeAtArg"
> 
> This still seems out-of-place...maybe it should be an introspective function of IntrinsicInst instead - "getTypeForArgNum"?
I believe this function means something like "Given an intrinsic which is isTriviallyVectorizable, but the intrinsic needs more than one type overload in the declaration (i.e not just the return type), this will return true for the operand number of the extra types to add. hasVectorInstrinsicScalarOpd will specify whether it needs to be scalar".

So I believe it's at least somewhat tied with the isTriviallyVectorizable method, and not general enough to handle all the intrinsics that could come up. At least that is my reading of it, it was added for https://reviews.llvm.org/D99439, but has been slightly changed by this patch.

I've renamed the functions, but with the second called isVectorIntrinsicWithOverloadTypeAtArg. Let me know if you think we should move it to IntrinsicInst.


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

https://reviews.llvm.org/D124358