On Mon, Apr 3, 2017 at 10:34 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> Hi!
>
> This patch deals just with correctness of vector shifts by scalar
> non-immediate. The manuals say the shift count is bits [0:63] of
> the corresponding source operand (XMM reg or memory in some cases),
> and if the count is bigger than number of bits - 1 in the vector element,
> it is treated as number of bits shift count.
> We are modelling it as SImode shift count though, the upper 32 bits
> may be random in some cases which causes wrong-code.
> Fixed by using DImode that matches what the insns do.
IIRC, SImode was choosen to simplify GPR->XMM register moves on 32bit
target. It does look this was wrong choice from the correctness point.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Any thoughts on what to do to generate reasonable code when the shift count
> comes from memory (e.g. as int variable) or is in the low bits of some XMM
> regioster?
The problem with int variable from memory is, that shifts access full
128bits for their count operand, so this is effectively a no-go. If
there is a 128bit count value in memory, we can maybe define shift
pattern with:
(subreg:DI (match_operand:V2DI 2 "general_operand" "xmN,vmN"))
?
> First of all, perhaps we could have some combiner (or peephole) pattern that
> would
> transform sign-extend from e.g. SI to DI on the shift count into zero-extend
> if there are no other uses of the extension result - if the shift count is
> negative in SImode (or even QImode), then it is already large number and the
> upper 32 bits or more don't really change anything on that.
We can introduce shift patterns with embedded extensions, and split
them to zext + shift. These new patterns can be easily macroized with
any_extend code iterator and SWI124 mode iterator, so we avoid pattern
explosion.
> Then perhaps we could emit pmovzxdq for SSE4.1+ instead of going through
> GPRs and back, or for SSE2 pxor on a scratch reg and punpck* to get it zero
> extended. Not sure if we want to add =v / vm alternative to
> zero_extendsidi2*, it already has some x but with ?s that prevent the RA
> from using it. So thoughts on that?
The ? is there to discourage RA from allocating xmm reg (all these
alternatives have * on xmm reg), in effect instructing RA to prefer
GPRs. If the value is already in xmm reg, then I expect ? alternative
will be used. So, yes, v/v alternative as you proposed would be a good
addition to zero_extendsidi alternatives. Please note though that
pmovzxdq operates on a vector value, so memory operands should be
avoided.
>
> 2017-04-03 Jakub Jelinek <ja...@redhat.com>
>
> PR target/80286
> * config/i386/i386.c (ix86_expand_args_builtin): If op has scalar
> int mode, convert_modes it to mode as unsigned, otherwise use
> lowpart_subreg to mode rather than SImode.
> * config/i386/sse.md (<mask_codefor>ashr<mode>3<mask_name>,
> ashr<mode>3, ashr<mode>3<mask_name>, <shift_insn><mode>3<mask_name>):
> Use DImode instead of SImode for the shift count operand.
> * config/i386/mmx.md (mmx_ashr<mode>3, mmx_<shift_insn><mode>3):
> Likewise.
> testsuite/
> * gcc.target/i386/avx-pr80286.c: New test.
> * gcc.dg/pr80286.c: New test.
OK for trunk and backports.
Thanks,
Uros.
> --- gcc/config/i386/i386.c.jj 2017-04-03 10:40:22.000000000 +0200
> +++ gcc/config/i386/i386.c 2017-04-03 18:31:39.482367634 +0200
> @@ -35582,10 +35582,17 @@ ix86_expand_args_builtin (const struct b
> {
> /* SIMD shift insns take either an 8-bit immediate or
> register as count. But builtin functions take int as
> - count. If count doesn't match, we put it in register. */
> + count. If count doesn't match, we put it in register.
> + The instructions are using 64-bit count, if op is just
> + 32-bit, zero-extend it, as negative shift counts
> + are undefined behavior and zero-extension is more
> + efficient. */
> if (!match)
> {
> - op = lowpart_subreg (SImode, op, GET_MODE (op));
> + if (SCALAR_INT_MODE_P (GET_MODE (op)))
> + op = convert_modes (mode, GET_MODE (op), op, 1);
> + else
> + op = lowpart_subreg (mode, op, GET_MODE (op));
> if (!insn_p->operand[i + 1].predicate (op, mode))
> op = copy_to_reg (op);
> }
> --- gcc/config/i386/sse.md.jj 2017-04-03 13:43:50.179572564 +0200
> +++ gcc/config/i386/sse.md 2017-04-03 18:01:19.713852914 +0200
> @@ -10620,7 +10620,7 @@ (define_insn "<mask_codefor>ashr<mode>3<
> [(set (match_operand:VI24_AVX512BW_1 0 "register_operand" "=v,v")
> (ashiftrt:VI24_AVX512BW_1
> (match_operand:VI24_AVX512BW_1 1 "nonimmediate_operand" "v,vm")
> - (match_operand:SI 2 "nonmemory_operand" "v,N")))]
> + (match_operand:DI 2 "nonmemory_operand" "v,N")))]
> "TARGET_AVX512VL"
> "vpsra<ssemodesuffix>\t{%2, %1, %0<mask_operand3>|%0<mask_operand3>, %1,
> %2}"
> [(set_attr "type" "sseishft")
> @@ -10634,7 +10634,7 @@ (define_insn "ashr<mode>3"
> [(set (match_operand:VI24_AVX2 0 "register_operand" "=x,x")
> (ashiftrt:VI24_AVX2
> (match_operand:VI24_AVX2 1 "register_operand" "0,x")
> - (match_operand:SI 2 "nonmemory_operand" "xN,xN")))]
> + (match_operand:DI 2 "nonmemory_operand" "xN,xN")))]
> "TARGET_SSE2"
> "@
> psra<ssemodesuffix>\t{%2, %0|%0, %2}
> @@ -10667,7 +10667,7 @@ (define_insn "ashr<mode>3<mask_name>"
> [(set (match_operand:VI248_AVX512BW_AVX512VL 0 "register_operand" "=v,v")
> (ashiftrt:VI248_AVX512BW_AVX512VL
> (match_operand:VI248_AVX512BW_AVX512VL 1 "nonimmediate_operand"
> "v,vm")
> - (match_operand:SI 2 "nonmemory_operand" "v,N")))]
> + (match_operand:DI 2 "nonmemory_operand" "v,N")))]
> "TARGET_AVX512F"
> "vpsra<ssemodesuffix>\t{%2, %1, %0<mask_operand3>|%0<mask_operand3>, %1,
> %2}"
> [(set_attr "type" "sseishft")
> @@ -10681,7 +10681,7 @@ (define_insn "<shift_insn><mode>3<mask_n
> [(set (match_operand:VI2_AVX2_AVX512BW 0 "register_operand" "=x,v")
> (any_lshift:VI2_AVX2_AVX512BW
> (match_operand:VI2_AVX2_AVX512BW 1 "register_operand" "0,v")
> - (match_operand:SI 2 "nonmemory_operand" "xN,vN")))]
> + (match_operand:DI 2 "nonmemory_operand" "xN,vN")))]
> "TARGET_SSE2 && <mask_mode512bit_condition> && <mask_avx512bw_condition>"
> "@
> p<vshift><ssemodesuffix>\t{%2, %0|%0, %2}
> @@ -10700,7 +10700,7 @@ (define_insn "<shift_insn><mode>3<mask_n
> [(set (match_operand:VI48_AVX2 0 "register_operand" "=x,x,v")
> (any_lshift:VI48_AVX2
> (match_operand:VI48_AVX2 1 "register_operand" "0,x,v")
> - (match_operand:SI 2 "nonmemory_operand" "xN,xN,vN")))]
> + (match_operand:DI 2 "nonmemory_operand" "xN,xN,vN")))]
> "TARGET_SSE2 && <mask_mode512bit_condition>"
> "@
> p<vshift><ssemodesuffix>\t{%2, %0|%0, %2}
> @@ -10720,7 +10720,7 @@ (define_insn "<shift_insn><mode>3<mask_n
> [(set (match_operand:VI48_512 0 "register_operand" "=v,v")
> (any_lshift:VI48_512
> (match_operand:VI48_512 1 "nonimmediate_operand" "v,m")
> - (match_operand:SI 2 "nonmemory_operand" "vN,N")))]
> + (match_operand:DI 2 "nonmemory_operand" "vN,N")))]
> "TARGET_AVX512F && <mask_mode512bit_condition>"
> "vp<vshift><ssemodesuffix>\t{%2, %1, %0<mask_operand3>|%0<mask_operand3>,
> %1, %2}"
> [(set_attr "isa" "avx512f")
> --- gcc/config/i386/mmx.md.jj 2017-04-03 13:43:50.119573339 +0200
> +++ gcc/config/i386/mmx.md 2017-04-03 18:01:19.708852979 +0200
> @@ -930,7 +930,7 @@ (define_insn "mmx_ashr<mode>3"
> [(set (match_operand:MMXMODE24 0 "register_operand" "=y")
> (ashiftrt:MMXMODE24
> (match_operand:MMXMODE24 1 "register_operand" "0")
> - (match_operand:SI 2 "nonmemory_operand" "yN")))]
> + (match_operand:DI 2 "nonmemory_operand" "yN")))]
> "TARGET_MMX"
> "psra<mmxvecsize>\t{%2, %0|%0, %2}"
> [(set_attr "type" "mmxshft")
> @@ -944,7 +944,7 @@ (define_insn "mmx_<shift_insn><mode>3"
> [(set (match_operand:MMXMODE248 0 "register_operand" "=y")
> (any_lshift:MMXMODE248
> (match_operand:MMXMODE248 1 "register_operand" "0")
> - (match_operand:SI 2 "nonmemory_operand" "yN")))]
> + (match_operand:DI 2 "nonmemory_operand" "yN")))]
> "TARGET_MMX"
> "p<vshift><mmxvecsize>\t{%2, %0|%0, %2}"
> [(set_attr "type" "mmxshft")
> --- gcc/testsuite/gcc.target/i386/avx-pr80286.c.jj 2017-04-03
> 18:44:07.552698281 +0200
> +++ gcc/testsuite/gcc.target/i386/avx-pr80286.c 2017-04-03 18:43:51.000000000
> +0200
> @@ -0,0 +1,26 @@
> +/* PR target/80286 */
> +/* { dg-do run { target avx } } */
> +/* { dg-options "-O2 -mavx" } */
> +
> +#include "avx-check.h"
> +#include <immintrin.h>
> +
> +__m256i m;
> +
> +__attribute__((noinline, noclone)) __m128i
> +foo (__m128i x)
> +{
> + int s = _mm_cvtsi128_si32 (_mm256_castsi256_si128 (m));
> + return _mm_srli_epi16 (x, s);
> +}
> +
> +static void
> +avx_test (void)
> +{
> + __m128i a = (__m128i) (__v8hi) { 1 << 7, 2 << 8, 3 << 9, 4 << 10, 5 << 11,
> 6 << 12, 7 << 13, 8 << 12 };
> + m = (__m256i) (__v8si) { 7, 8, 9, 10, 11, 12, 13, 14 };
> + __m128i c = foo (a);
> + __m128i b = (__m128i) (__v8hi) { 1, 2 << 1, 3 << 2, 4 << 3, 5 << 4, 6 <<
> 5, 7 << 6, 8 << 5 };
> + if (__builtin_memcmp (&c, &b, sizeof (__m128i)))
> + __builtin_abort ();
> +}
> --- gcc/testsuite/gcc.dg/pr80286.c.jj 2017-04-03 18:45:27.574663948 +0200
> +++ gcc/testsuite/gcc.dg/pr80286.c 2017-04-03 18:45:18.386782707 +0200
> @@ -0,0 +1,23 @@
> +/* PR target/80286 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wno-psabi" } */
> +
> +typedef int V __attribute__((vector_size (4 * sizeof (int))));
> +
> +__attribute__((noinline, noclone)) V
> +foo (V x, V y)
> +{
> + return x << y[0];
> +}
> +
> +int
> +main ()
> +{
> + V x = { 1, 2, 3, 4 };
> + V y = { 5, 6, 7, 8 };
> + V z = foo (x, y);
> + V e = { 1 << 5, 2 << 5, 3 << 5, 4 << 5 };
> + if (__builtin_memcmp (&z, &e, sizeof (V)))
> + __builtin_abort ();
> + return 0;
> +}
>
> Jakub
