[PATCH] Fix for previously reverted SLPVectorizer cost model improvement

[Arnold Schwaighofer]

Can you change the comment to:


+    /// This method returns true if the intrinsic's argument types are all scalars


from:


+    /// Method returns true if intrinsic arguments types are all scalars
+    /// for the scalar form of the intrinsic and all vectors for the vector
+    /// form of the intrinsic.



with this change this LGTM.



On 04/04/14, Tyler Nowicki   wrote:
> Thanks for the review
> 
> Here is an updated patch. I made the changes you asked for including splitting out the unrelated code, modifying the comment, removing the lifetime intrinsics. I also adding ctpop which was previously missing.
> 
> Tyler
> 
> 
> 
> 
> 
> On Apr 3, 2014, at 4:30 PM, arnold <aschwaighofer at apple.com> wrote:
> 
> > --- include/llvm/IR/IntrinsicInst.h	(revision 205571)
> > +++ include/llvm/IR/IntrinsicInst.h	(working copy)
> > @@ -44,6 +44,38 @@
> >       return (Intrinsic::ID)getCalledFunction()->getIntrinsicID();
> >     }
> > 
> > +    /// Method returns true if any intrinsic argument types are scalar
> > +    /// in both vector and scalar forms of the intrisinc.
> > 
> > The other way round (except for lifetime_start). Like you describe below in the explanation of the patch.
> > 
> > +    ///
> > +    static inline bool isTriviallyVectorizable(Intrinsic::ID ID) {
> > +      switch (ID) {
> > +        case Intrinsic::sort:
> > ...
> > +        case Intrinsic::lifetime_start:
> > +        case Intrinsic::lifetime_end:
> > +          return true;
> > +        default:
> > +          return false;
> > +      }
> > +    }
> > 
> > Because of Intrinsic::lifetime_start the comment is not totally true. A lifetime_start/end is vectorizable because we simple scalarize it in the loop vectorizer. For all other intrinsics we can vectorize them because all their arguments become vectors when vectorized (like you mention below). I think lifetime_start/stop should be handled separately (not be checked as part of isTriviallyVectorizable).
> > 
> > Can you separate the patch into two patches? One that corrects the handling of mixed argument intrinsic and the second one reverting my revert.
> > 
> > You can test the first patch by moving both insertelement instructions in the example non-vectorizable-intrinsic.ll before the return.
> > 
> > Thanks for working on this!
> > 
> > On Apr 3, 2014, at 2:44 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
> > 
> >> Hi,
> >> 
> >> This patch modifies r205260 which was previously reverted by r205018 because it a crash. The original patch by Arch Robison and Arnold Schwaighofer.
> >> 
> >> This patch stops the SLP vectorizer from trying to vectorize a call that is not trivially vectorizable. An intrinsic is not trivially vectorized if any of the arguments cannot be simply packed together into a tuple. For example, the call to CTLZ cannot be trivially replaced with a vector intrinsic because the second parameter is scalar in both its scalar and vector forms.
> >> 
> >> Tyler Nowicki
> >> 
> >> <slp_insertelements_fix2.patch>_______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >