[PATCH] Fix for previously reverted SLPVectorizer cost model improvement

[Eric Christopher]

Seems reasonable to me. No strong preference. :)

Thanks for the work!

-eric
On Apr 8, 2014 5:03 PM, "Tyler Nowicki" <tnowicki at apple.com> wrote:

> Thanks for your review!
>
> There doesn't seem to be a place for vectorization utils. How about
> creating one similar to LoopUtils.h?
>
> include/llvm/Transforms/Utils/VectorUtils.h
>
> Does that sound more reasonable?
>
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140408/2e55fded/attachment.html>