[x86] Avoid builtins for SSE/AVX2 immidiate logical shifts

On Saturday 22 April 2017, Allan Sandfeld Jensen wrote:
> Replaces definitions of immediate logical shift intrinsics with GCC
> extension syntax. Tests are added to ensure the intrinsics still produce
> the right instructions and that a few basic optimizations now work.
> 
> Compared to the earlier version of the patch, all potentially undefined
> shifts are now avoided, which also means no variable shifts or arithmetic
> right shifts.

Fixed 2 errors in the tests.

On Mon, Apr 24, 2017 at 09:33:09AM +0200, Allan Sandfeld Jensen wrote:
> --- a/gcc/config/i386/avx2intrin.h
> +++ b/gcc/config/i386/avx2intrin.h
> @@ -667,7 +667,7 @@ extern __inline __m256i
>  __attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
>  _mm256_slli_epi16 (__m256i __A, int __B)
>  {
> -  return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)__A, __B);
> +  return ((__B & 0xff) < 16) ? (__m256i)((__v16hi)__A << (__B & 0xff)) : _mm256_setzero_si256();
>  }

What is the advantage of doing that when you replace one operation with
several (&, <, ?:, <<)?
I'd say instead we should fold the builtins if in the gimple fold target
hook we see the shift count constant and can decide based on that.
Or we could use __builtin_constant_p (__B) to decide whether to use
the generic vector shifts or builtin, but that means larger IL.

	Jakub

On Monday 24 April 2017, Jakub Jelinek wrote:
> On Mon, Apr 24, 2017 at 09:33:09AM +0200, Allan Sandfeld Jensen wrote:
> > --- a/gcc/config/i386/avx2intrin.h
> > +++ b/gcc/config/i386/avx2intrin.h
> > @@ -667,7 +667,7 @@ extern __inline __m256i
> > 
> >  __attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
> >  _mm256_slli_epi16 (__m256i __A, int __B)
> >  {
> > 
> > -  return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)__A, __B);
> > +  return ((__B & 0xff) < 16) ? (__m256i)((__v16hi)__A << (__B & 0xff)) :
> > _mm256_setzero_si256();
> > 
> >  }
> 
> What is the advantage of doing that when you replace one operation with
> several (&, <, ?:, <<)?
> I'd say instead we should fold the builtins if in the gimple fold target
> hook we see the shift count constant and can decide based on that.
> Or we could use __builtin_constant_p (__B) to decide whether to use
> the generic vector shifts or builtin, but that means larger IL.

The advantage is that in this builtin, the __B is always a literal (or 
constexpr), so the if statement is resolved at compile time.

`Allan

On Mon, Apr 24, 2017 at 09:51:29AM +0200, Allan Sandfeld Jensen wrote:
> On Monday 24 April 2017, Jakub Jelinek wrote:
> > On Mon, Apr 24, 2017 at 09:33:09AM +0200, Allan Sandfeld Jensen wrote:
> > > --- a/gcc/config/i386/avx2intrin.h
> > > +++ b/gcc/config/i386/avx2intrin.h
> > > @@ -667,7 +667,7 @@ extern __inline __m256i
> > > 
> > >  __attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
> > >  _mm256_slli_epi16 (__m256i __A, int __B)
> > >  {
> > > 
> > > -  return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)__A, __B);
> > > +  return ((__B & 0xff) < 16) ? (__m256i)((__v16hi)__A << (__B & 0xff)) :
> > > _mm256_setzero_si256();
> > > 
> > >  }
> > 
> > What is the advantage of doing that when you replace one operation with
> > several (&, <, ?:, <<)?
> > I'd say instead we should fold the builtins if in the gimple fold target
> > hook we see the shift count constant and can decide based on that.
> > Or we could use __builtin_constant_p (__B) to decide whether to use
> > the generic vector shifts or builtin, but that means larger IL.
> 
> The advantage is that in this builtin, the __B is always a literal (or 
> constexpr), so the if statement is resolved at compile time.

Do we really want to support all the thousands _mm* intrinsics in constexpr
contexts?  People can just use generic vectors instead.

That said, both the options I've mentioned above provide the same advantages
and don't have the disadvantages of pessimizing normal code.

	Jakub

On Monday 24 April 2017, Jakub Jelinek wrote:
> On Mon, Apr 24, 2017 at 09:51:29AM +0200, Allan Sandfeld Jensen wrote:
> > On Monday 24 April 2017, Jakub Jelinek wrote:
> > > On Mon, Apr 24, 2017 at 09:33:09AM +0200, Allan Sandfeld Jensen wrote:
> > > > --- a/gcc/config/i386/avx2intrin.h
> > > > +++ b/gcc/config/i386/avx2intrin.h
> > > > @@ -667,7 +667,7 @@ extern __inline __m256i
> > > > 
> > > >  __attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
> > > >  _mm256_slli_epi16 (__m256i __A, int __B)
> > > >  {
> > > > 
> > > > -  return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)__A, __B);
> > > > +  return ((__B & 0xff) < 16) ? (__m256i)((__v16hi)__A << (__B &
> > > > 0xff)) : _mm256_setzero_si256();
> > > > 
> > > >  }
> > > 
> > > What is the advantage of doing that when you replace one operation with
> > > several (&, <, ?:, <<)?
> > > I'd say instead we should fold the builtins if in the gimple fold
> > > target hook we see the shift count constant and can decide based on
> > > that. Or we could use __builtin_constant_p (__B) to decide whether to
> > > use the generic vector shifts or builtin, but that means larger IL.
> > 
> > The advantage is that in this builtin, the __B is always a literal (or
> > constexpr), so the if statement is resolved at compile time.
> 
> Do we really want to support all the thousands _mm* intrinsics in constexpr
> contexts?  People can just use generic vectors instead.
> 
I would love to support it, but first we need a C extension attribute matching 
constexpr, and I consider it a separate issue.

> That said, both the options I've mentioned above provide the same
> advantages and don't have the disadvantages of pessimizing normal code.
> 
What pessimizing? This produce the same or better code for all legal 
arguments. The only difference besides better generated code is that it allows 
the intrinsics to be used incorrectly with non-literal arguments because we 
lack the C-extension for constexp to prevent that.

`Allan

On Mon, Apr 24, 2017 at 10:02:40AM +0200, Allan Sandfeld Jensen wrote:
> > That said, both the options I've mentioned above provide the same
> > advantages and don't have the disadvantages of pessimizing normal code.
> > 
> What pessimizing? This produce the same or better code for all legal 
> arguments. The only difference besides better generated code is that it allows 

No.  Have you really tried that?

> the intrinsics to be used incorrectly with non-literal arguments because we 
> lack the C-extension for constexp to prevent that.

Consider e.g. -O2 -mavx2 -mtune=intel:
#include <x86intrin.h>

__m256i
foo (__m256i x, int s)
{
  return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)x, s);
}

__m256i
bar (__m256i x, int s)
{
  return ((s & 0xff) < 16) ? (__m256i)((__v16hi)x << (s & 0xff)) : _mm256_setzero_si256 ();
}

The first one generates
        movl    %edi, %edi
        vmovq   %rdi, %xmm1
        vpsllw  %xmm1, %ymm0, %ymm0
        ret
(because that is actually what the instruction does), the second one
        movzbl  %dil, %edi
        cmpl    $15, %edi
        jg      .L5
        vmovq   %rdi, %xmm1
        vpsllw  %xmm1, %ymm0, %ymm0
        ret
        .p2align 4,,7
        .p2align 3
.L5:
        vpxor   %xmm0, %xmm0, %xmm0
        ret

	Jakub

On Monday 24 April 2017, Jakub Jelinek wrote:
> On Mon, Apr 24, 2017 at 10:02:40AM +0200, Allan Sandfeld Jensen wrote:
> > > That said, both the options I've mentioned above provide the same
> > > advantages and don't have the disadvantages of pessimizing normal code.
> > 
> > What pessimizing? This produce the same or better code for all legal
> > arguments. The only difference besides better generated code is that it
> > allows
> 
> No.  Have you really tried that?
> 
> > the intrinsics to be used incorrectly with non-literal arguments because
> > we lack the C-extension for constexp to prevent that.
> 
> Consider e.g. -O2 -mavx2 -mtune=intel:
> #include <x86intrin.h>
> 
> __m256i
> foo (__m256i x, int s)
> {
>   return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)x, s);
> }
> 
> __m256i
> bar (__m256i x, int s)
> {
>   return ((s & 0xff) < 16) ? (__m256i)((__v16hi)x << (s & 0xff)) :
> _mm256_setzero_si256 (); }
> 
> The first one generates
>         movl    %edi, %edi
>         vmovq   %rdi, %xmm1
>         vpsllw  %xmm1, %ymm0, %ymm0
>         ret
> (because that is actually what the instruction does), the second one
That is a different instruction. That is the vpsllw not vpsllwi

The intrinsics I changed is the immediate version, I didn't change the non-
immediate version. It is probably a bug if you can give non-immediate values 
to the immediate only intrinsic. At least both versions handles it, if in 
different ways, but is is illegal arguments.

`Allan

On Monday 24 April 2017, Allan Sandfeld Jensen wrote:
> On Monday 24 April 2017, Jakub Jelinek wrote:
> > On Mon, Apr 24, 2017 at 10:02:40AM +0200, Allan Sandfeld Jensen wrote:
> > > > That said, both the options I've mentioned above provide the same
> > > > advantages and don't have the disadvantages of pessimizing normal
> > > > code.
> > > 
> > > What pessimizing? This produce the same or better code for all legal
> > > arguments. The only difference besides better generated code is that it
> > > allows
> > 
> > No.  Have you really tried that?
> > 
> > > the intrinsics to be used incorrectly with non-literal arguments
> > > because we lack the C-extension for constexp to prevent that.
> > 
> > Consider e.g. -O2 -mavx2 -mtune=intel:
> > #include <x86intrin.h>
> > 
> > __m256i
> > foo (__m256i x, int s)
> > {
> > 
> >   return (__m256i)__builtin_ia32_psllwi256 ((__v16hi)x, s);
> > 
> > }
> > 
> > __m256i
> > bar (__m256i x, int s)
> > {
> > 
> >   return ((s & 0xff) < 16) ? (__m256i)((__v16hi)x << (s & 0xff)) :
> > _mm256_setzero_si256 (); }
> > 
> > The first one generates
> > 
> >         movl    %edi, %edi
> >         vmovq   %rdi, %xmm1
> >         vpsllw  %xmm1, %ymm0, %ymm0
> >         ret
> > 
> > (because that is actually what the instruction does), the second one
> 
> That is a different instruction. That is the vpsllw not vpsllwi
> 
> The intrinsics I changed is the immediate version, I didn't change the non-
> immediate version. It is probably a bug if you can give non-immediate
> values to the immediate only intrinsic. At least both versions handles it,
> if in different ways, but is is illegal arguments.
> 
Though I now that I think about it, this means my change of to the existing 
sse-psslw-1.c test and friends is wrong, because it uses variable input.

`Allan

On Mon, Apr 24, 2017 at 10:34:58AM +0200, Allan Sandfeld Jensen wrote:
> That is a different instruction. That is the vpsllw not vpsllwi
> 
> The intrinsics I changed is the immediate version, I didn't change the non-
> immediate version. It is probably a bug if you can give non-immediate values 
> to the immediate only intrinsic. At least both versions handles it, if in 
> different ways, but is is illegal arguments.

The documentation is unclear on that and I've only recently fixed up some
cases where these intrinsics weren't able to handle non-constant arguments
in some cases, while both ICC and clang coped with that fine.
So it is clearly allowed and handled by all the compilers and needs to be
supported, people use that in real-world code.

	Jakub

On Mon, Apr 24, 2017 at 11:01:29AM +0200, Allan Sandfeld Jensen wrote:
> On Monday 24 April 2017, Jakub Jelinek wrote:
> > On Mon, Apr 24, 2017 at 10:34:58AM +0200, Allan Sandfeld Jensen wrote:
> > > That is a different instruction. That is the vpsllw not vpsllwi
> > > 
> > > The intrinsics I changed is the immediate version, I didn't change the
> > > non- immediate version. It is probably a bug if you can give
> > > non-immediate values to the immediate only intrinsic. At least both
> > > versions handles it, if in different ways, but is is illegal arguments.
> > 
> > The documentation is unclear on that and I've only recently fixed up some
> > cases where these intrinsics weren't able to handle non-constant arguments
> > in some cases, while both ICC and clang coped with that fine.
> > So it is clearly allowed and handled by all the compilers and needs to be
> > supported, people use that in real-world code.
> > 
> Undoubtedly it happens. I just make a mistake myself that created that case. 
> But it is rather unfortunate, and means we make wrong code currently for 
> corner case values.

The intrinsic documentation is poor, usually you have a good documentation
on what the instructions do, and then you just have to guess what the
intrinsics do.  You can of course ask Intel for clarification.

If you try:
#include <x86intrin.h>

__m128i
foo (__m128i a, int b)
{
  return _mm_slli_epi16 (a, b);
}
and call it with 257 from somewhere else, you can see that all the compilers
will give you zero vector.  And similarly if you use 257 literally instead
of b.  So what the intrinsic (unlike the instruction)
actually does is that it compares all bits of the imm8 argument (supposedly
using unsigned comparison) and if it is bigger than 15 (or 7 or 31 or 63
depending on the bitsize of element) it yields 0 vector.

	Jakub

On Monday 24 April 2017, Jakub Jelinek wrote:
> On Mon, Apr 24, 2017 at 11:01:29AM +0200, Allan Sandfeld Jensen wrote:
> > On Monday 24 April 2017, Jakub Jelinek wrote:
> > > On Mon, Apr 24, 2017 at 10:34:58AM +0200, Allan Sandfeld Jensen wrote:
> > > > That is a different instruction. That is the vpsllw not vpsllwi
> > > > 
> > > > The intrinsics I changed is the immediate version, I didn't change
> > > > the non- immediate version. It is probably a bug if you can give
> > > > non-immediate values to the immediate only intrinsic. At least both
> > > > versions handles it, if in different ways, but is is illegal
> > > > arguments.
> > > 
> > > The documentation is unclear on that and I've only recently fixed up
> > > some cases where these intrinsics weren't able to handle non-constant
> > > arguments in some cases, while both ICC and clang coped with that
> > > fine.
> > > So it is clearly allowed and handled by all the compilers and needs to
> > > be supported, people use that in real-world code.
> > 
> > Undoubtedly it happens. I just make a mistake myself that created that
> > case. But it is rather unfortunate, and means we make wrong code
> > currently for corner case values.
> 
> The intrinsic documentation is poor, usually you have a good documentation
> on what the instructions do, and then you just have to guess what the
> intrinsics do.  You can of course ask Intel for clarification.
> 
> If you try:
> #include <x86intrin.h>
> 
> __m128i
> foo (__m128i a, int b)
> {
>   return _mm_slli_epi16 (a, b);
> }
> and call it with 257 from somewhere else, you can see that all the
> compilers will give you zero vector.  And similarly if you use 257
> literally instead of b.  So what the intrinsic (unlike the instruction)
> actually does is that it compares all bits of the imm8 argument (supposedly
> using unsigned comparison) and if it is bigger than 15 (or 7 or 31 or 63
> depending on the bitsize of element) it yields 0 vector.
> 
Good point. I was using intel's documentation at 
https://software.intel.com/sites/landingpage/IntrinsicsGuide/, but if all 
compilers including us does something else, practicality wins.

It did make me curious and test out what _mm_slli_epi16(v, -250); compiles to. 
For some reason that becomes an undefined shift using the non-immediate sll in 
gcc, but returns the zero-vector in clang. With my patch it was a 6 bit shift, 
but that is apparently not de-facto standard.


`Allan