[PATCH] D57090: [IR] Match intrinsic paramater by scalar/vectorwidth

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

In D57090#1367749 <https://reviews.llvm.org/D57090#1367749>, @RKSimon wrote:

> In D57090#1367719 <https://reviews.llvm.org/D57090#1367719>, @spatel wrote:
>
> > This implies that we should change the LangRef for overflowing ops to match? Currently, there's no mention of vector types:
> >  http://llvm.org/docs/LangRef.html#arithmetic-with-overflow-intrinsics
>
>
> If its alright, I'd prefer to wait on updating the docs until I have full testing of vector types in place, which would be in a followup patch.


Yes, that sounds ok to me. LGTM - see inline for a couple of nits.



================
Comment at: include/llvm/IR/Intrinsics.td:163
+// number of elements.
+class LLVMScalarOrSameVectorWidth<int num, LLVMType elty>
   : LLVMMatchType<num> {
----------------
Not sure if I've ever looked at this code before, so it wasn't immediately clear what 'num' was referring to. Would it make sense to name that "index" or "opIndex"? (Similarly in the existing code around this).


================
Comment at: lib/IR/Function.cpp:951
     Type *Ty = Tys[D.getArgumentNumber()];
-    if (VectorType *VTy = dyn_cast<VectorType>(Ty)) {
+    if (VectorType *VTy = dyn_cast<VectorType>(Ty))
       return VectorType::get(EltTy, VTy->getNumElements());
----------------
use 'auto' for consistency with the other dyn_casts around here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57090