Re: [PATCH] Inefficient code generation for 128-bit-≥256-bittypecast intrinsics (BZ #15712)

[Craig Topper]

Option (2) directly matches the capabilities of the shufflevector
instruction in the LLVM IR. I have attached a patch that will allow -1 to
become undef in the IR.

So

__builtin_shufflevector( x, y, 0, 4, -1, 5 );

becomes

shufflevector <4 x float> %x, <4 x float> %y, <4 x i32> <i32 0, i32 4, i32
undef, i32 5>


On Fri, Aug 2, 2013 at 6:15 PM, Katya Romanova <
katya_romanova at playstation.sony.com> wrote:

>
>
> Craig Topper <craig.topper at ...> writes:
>
> >
> >
> > Ok so -1 isn't valid for indices, and i have even more questions about
> __builtin_shufflevector the more i look at it. See my message in cfe-dev.
> >
> >
> > On Thu, Jul 18, 2013 at 6:12 PM, Chandler Carruth
> <chandlerc at google.com> wrote:
> >
> > On Thu, Jul 18, 2013 at 6:11 PM, Craig Topper
> <craig.topper at gmail.com> wrote:
> >
> >
> >
> >
> >
> >
> > Would __builtin_shufflevector(__a, __a, 0, 1, -1, -1)  work?
> >
> >
> >
> >
> >
> > Personally, I would prefer a defined way to produce an undef input in
> general... but if folks are worried about exposing such an interface, then
> sure, we could just allow the shuffle builtin itself to designate an
> "undef"
> input with goofy indices.
> >
> >
> >
> >
> >
> >
> > On Thu, Jul 18, 2013 at 5:42 PM, Chandler Carruth
> <chandlerc at google.com> wrote:
> >
> >
> >
> >
> >
> >
> >
> > On Thu, Jul 18, 2013 at 5:32 PM, Katya Romanova
> <Katya_Romanova at playstation.sony.com> wrote:-
>  __m128d __zero = _mm_setzero_pd();
> > -  return __builtin_shufflevector(__a, __zero, 0, 1, 2, 2);
> > +  return (__m256d)__builtin_ia32_pd256_pd((__v2df)__a);
> >
> >
> > I think this is the wrong approach.
> >
> > Rather than switching these to use an x86-specific builtin, instead it
> would be better to provide some generic form to produce an undef input to a
> shufflevector. That is a generally useful and completely target independent
> concept.
> >
> >
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing listcfe-commits <at>
> cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
> > -- ~Craig
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > -- ~Craig
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at ...
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
>
>
> I agree with Chandler that it's better to use a shuffle with undef input
> (which is target independent), even though we generate code for AVX
> intrinsics. The reason I initially ended up using a x86-specific builtin is
> because there I couldn't find a generic way to create "undef" input for a
> shuffle.
>
> I tried the following, but I didn't like it, because the compiler gives a
> warning when compiling avxintrin.h
>
> static __inline __m256d __attribute__((__always_inline__, __nodebug__))
> _mm256_castpd128_pd256(__m128d in)
> {
>   __m128d undef;
>   return __builtin_shufflevector(in, undef, 0, 1, 2, 2);
> }
>
> I tried this as well and I didn't like it either:
>
> static __inline __m256d __attribute__((__always_inline__, __nodebug__))
> _mm256_castpd128_pd256(__m128d in)
> {
>   __v2df __in = (__v2df) in;
>   __v4df ret;
>   ret[0]=in[0];
>   ret[1]=in[1];
>   return (__m256d)ret;
> }
>
> So, I ended up introducing a x86_64 builtin and lowered it later to a
> shuffle with undef (not a target-independent solution).
>
> static __inline __m256d __attribute__((__always_inline__, __nodebug__))
>  _mm256_castpd128_pd256(__m128d __a)  {
>   return (__m256d)__builtin_ia32_pd256_pd((__v2df)__a);
> }
>
>
> I've read Craig's proposal about using shuffle builtin with negative
> indeces
> (-1) to indicate shuffle with undef. This solution looks good. However,
> "-1"
> shuffle index is presently considered invalid. We need to discuss extending
> shuffle syntax/semantics and then implement this extension before I could
> use a shuffle with negative indices for AVX typecast builtins. It looks
> like
> it will take some time...
>
> I was wondering if it's possible to check in my current fix that is using
> x86_86 builtins (instead of a shuffle) for AVX typecast intrinsics for now.
> When shuffle learns to understand negative indices, I could easily replaces
> my changes with something like that:
>
> __builtin_shufflevector(__a, __a, 0, 1, -1, -1)
>
> If this interim solution doesn’t sound inappropriate, we should start a
> discussion about extending shuffle builtin functionality to understand
> negative indexes.
>
> Here are several ideas:
>
> We could use "unary" form of __builtin_shufflevector when negative indices
> are used.
> A "binary" form could be used with negive indexes as well, but semantic
> analysis should ensure that the first and the second parameter is actually
> the same vector. Here is the reason for this limitation:
>
> If negative indices specify "undef" and a binary form of
> __builtin_shufflevector is used with different first and second parameter,
> e.g. __builtin_shufflevector(a, b, 0, 1, 7, -1)
> then, in fact, we will be shuffling 3 vectors (a, b and undef). I don’t
> think that it’s a good idea to extend __builtin_shufflevector semantic to
> do
> that.
>
>
>  Which solution is preferred?
> (1) Support negative indices for unary form of __builtin_shufflevector
> only.
> (2) Support negative indices for binary form of __builtin_shufflevector
> only
> and ensure that the first and the second parameter is the same vector.
> (3) Support both (1) and (2).
> (4) Another possible (though very different from proposed above) solution
> that allows to use "undef" in shuffles would be adding a target-independent
> builtin (e.g __builtin_undef(vector a)), which creates an “undef” vector
> with the same type and the same number of elements as its vector argument.
> With this "undef" builtin, I could implement AVX typecast builtins like
> that:
>
> static __inline __m256d __attribute__((__always_inline__, __nodebug__))
> _mm256_castpd128_pd256(__m128d in)
> {
>   __m128d undef = __builtin_undef(in);
>   return __builtin_shufflevector(in, undef, 0, 1, 2, 2);
> }
>
> Thoughts?
>
>
> Thank you!
> Katya.
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>



-- 
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130803/0038f5ec/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shuffle_undef.patch
Type: application/octet-stream
Size: 2130 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130803/0038f5ec/attachment.obj>