[PATCH] Fix for previously reverted SLPVectorizer cost model improvement

[Arnold]

Yes, committed as 205855.

> On Apr 9, 2014, at 10:47 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
> Looks like this got committed? (No problem on my end, just closing out
> the thread in my inbox after my flight back)
> 
> -eric
> 
>> On Tue, Apr 8, 2014 at 8:22 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
>> Thanks for the review!
>> 
>> For lack of a better place I moved isTriviallyVectorizable() to
>> VectorUtils.h and I left the function name as is.
>> 
>> Here is the updated patch.
>> 
>> Tyler
>> 
>> 
>> 
>> On Apr 8, 2014, at 7:50 AM, Eric Christopher <echristo at gmail.com> wrote:
>> 
>> On Mon, Apr 7, 2014 at 6:34 PM, Arnold Schwaighofer
>> <aschwaighofer at apple.com> wrote:
>> 
>> 
>> On Apr 6, 2014, at 9:14 PM, Nadav Rotem <nrotem at apple.com> wrote:
>> 
>> +    static inline bool isTriviallyVectorizable(Intrinsic::ID ID) {
>> 
>> I don't think that this function belongs in include/llvm/IR/IntrinsicInst.h.
>> This function describes the current capabilities of the loop vectorizer.
>> Would you add a similar function that describes functions that can be
>> LICM-ed or Scalarized or Unrolled into Intrinsics.h?  Moreover, we could
>> easily teach the vectorizer to vectorize these functions (we have done it
>> with Intel's OpenCL vectorizer).
>> 
>> 
>> I think this api describes a property of the intrinsic instruction: Whether
>> all operands are vector operands of the same width when the instruction is
>> vectorized. Maybe, we should name it such: areAllOpdsIdenticallyWidened?
>> 
>> It is used by the two vectorizers to determine whether they can vectorize
>> the instruction by simply widening the types:
>> 
>> float @llvm.pow.f32(float  %Val, i32 %power) => <2 x float>
>> @llvm.pow.f32(<2 x float> %Val, <2 x i32> %power)
>> 
>> in contrast to:
>> 
>> float @llvm.powi.f32(float  %Val, i32 %power) => <2 x float>
>> @llvm.powi.f32(<2 x float> %Val, i32 %power)
>> 
>> 
>> I think I agree with Nadav here. It's a piece of knowledge that's not
>> generally useful outside of the vectorizer pass and there's really no
>> reason for it to be in Intrinsics.h. It describes a property, but we
>> should only put things in Intrinsics.h if they describe properties on
>> the intrinsic definition.
>> 
>> Thoughts?
>> 
>> -eric
>> 
>> 
>> Best,
>> Arnold
>> 
>> 
>> 
>> 
>> On Apr 6, 2014, at 5:47 PM, Arnold Schwaighofer <aschwaighofer at apple.com>
>> wrote:
>> 
>> 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
>> 
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>>