On Mon, Feb 11, 2019 at 2:10 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Sat, Feb 09, 2019 at 02:39:30PM +0100, Jakub Jelinek wrote:
> > On Sat, Feb 09, 2019 at 01:22:30PM +0100, Jakub Jelinek wrote:
> > > On Sat, Feb 09, 2019 at 04:11:43AM -0800, H.J. Lu wrote:
> > > > I believe all usages of
> > > >
> > > > (ior (match_operand 0 "ext_sse_reg_operand")
> > > > (match_operand 1 "ext_sse_reg_operand"))
> > > >
> > > > should be checked. I am not sure if they should be there at all.
> > >
> > > E.g. in i386.md all the other spots look fine, because {DI,SI,DF,SF}mode
> > > is allowed in ext sse regs even with -mavx512f. And sse.md doesn't use
> > > this
> > > at all. What I'm wondering is if we need the sse.md (*mov<mode>_internal)
> > > code I've cited earlier, doing bootstrap/regtest now with gcc_unreachable
> > > in
> > > there (and in *mov{o,x}i_internal* for MODE_XI too) too see if it ever
> > > triggers.
> >
> > The following didn't ICE on anything, which is not a proof, but given that
> > hard_regno_mode_ok should return false for ext_sse_reg_operand regs for
> > avx512f && !avx512vl, it matches my expectations, on the other hand, it was
> > a normal defaults bootstrap, don't have a knl which might be best for this
> > to test -mavx512f -mno-avx512vl on everything.
> > So perhaps we can also nuke the large if from mov<mode>_internal.
> >
> > --- gcc/config/i386/i386.md.jj 2019-02-09 12:35:57.971475641 +0100
> > +++ gcc/config/i386/i386.md 2019-02-09 12:37:40.776802962 +0100
> > @@ -1905,6 +1905,7 @@ (define_insn "*movoi_internal_avx"
> > return standard_sse_constant_opcode (insn, operands);
> >
> > case TYPE_SSEMOV:
> > + gcc_assert (get_attr_mode (insn) != MODE_XI);
> > if (misaligned_operand (operands[0], OImode)
> > || misaligned_operand (operands[1], OImode))
> > {
> > @@ -1970,6 +1971,7 @@ (define_insn "*movti_internal"
> > case TYPE_SSEMOV:
> > /* TDmode values are passed as TImode on the stack. Moving them
> > to stack may result in unaligned memory access. */
> > + gcc_assert (get_attr_mode (insn) != MODE_XI);
> > if (misaligned_operand (operands[0], TImode)
> > || misaligned_operand (operands[1], TImode))
> > {
> > --- gcc/config/i386/sse.md.jj 2019-01-28 21:57:39.301110220 +0100
> > +++ gcc/config/i386/sse.md 2019-02-09 12:36:45.863696416 +0100
> > @@ -989,6 +989,7 @@ (define_insn "mov<mode>_internal"
> > && (EXT_REX_SSE_REG_P (operands[0])
> > || EXT_REX_SSE_REG_P (operands[1])))
> > {
> > + gcc_unreachable ();
> > if (memory_operand (operands[0], <MODE>mode))
> > {
> > if (<MODE_SIZE> == 32)
> >
>
> Here is the updated patch to remove ext_sse_reg_operand check with a
> testcase.
>
> OK for trunk?
No. As said, please correctly set mode to XImode in mode attribute calculation.
Uros.
> Thanks.
>
> H.J.
> ---
> Since hard_regno_mode_ok only allows xmm16-xmm31 in OImode or TImode
> with TARGET_AVX512VL:
>
> /* AVX512VL allows sse regs16+ for 128/256 bit modes. */
> if (TARGET_AVX512VL
> && (mode == OImode
> || mode == TImode
> || VALID_AVX256_REG_MODE (mode)
> || VALID_AVX512VL_128_REG_MODE (mode)))
> return true;
>
> /* xmm16-xmm31 are only available for AVX-512. */
> if (EXT_REX_SSE_REGNO_P (regno))
> return false;
>
> there is no need to check ext_sse_reg_operand in *movoi_internal_avx nor
> *movti_internal. Instead, we should check EXT_REX_SSE_REG_P for upper 16
> vector registers.
>
> 2019-02-11 H.J. Lu <hongjiu...@intel.com>
> Jakub Jelinek <ja...@redhat.com>
>
> gcc/
>
> PR target/89229
> * config/i386/i386.md (*movoi_internal_avx): Check
> EXT_REX_SSE_REG_P instead of MODE_XI for upper 16 vector
> registers.
> (*movti_internal): Likewise.
>
> gcc/testsuite/
>
> PR target/89229
> * gcc.target/i386/pr89229-1.c: New test.
> ---
> gcc/config/i386/i386.md | 22 +++++------
> gcc/testsuite/gcc.target/i386/pr89229-1.c | 47 +++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 13 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr89229-1.c
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 3d9141ae450..5b89e52493e 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -1910,7 +1910,8 @@
> {
> if (get_attr_mode (insn) == MODE_V8SF)
> return "vmovups\t{%1, %0|%0, %1}";
> - else if (get_attr_mode (insn) == MODE_XI)
> + else if (EXT_REX_SSE_REG_P (operands[0])
> + || EXT_REX_SSE_REG_P (operands[1]))
> return "vmovdqu32\t{%1, %0|%0, %1}";
> else
> return "vmovdqu\t{%1, %0|%0, %1}";
> @@ -1919,7 +1920,8 @@
> {
> if (get_attr_mode (insn) == MODE_V8SF)
> return "vmovaps\t{%1, %0|%0, %1}";
> - else if (get_attr_mode (insn) == MODE_XI)
> + else if (EXT_REX_SSE_REG_P (operands[0])
> + || EXT_REX_SSE_REG_P (operands[1]))
> return "vmovdqa32\t{%1, %0|%0, %1}";
> else
> return "vmovdqa\t{%1, %0|%0, %1}";
> @@ -1933,11 +1935,7 @@
> (set_attr "type" "sselog1,sselog1,ssemov,ssemov")
> (set_attr "prefix" "vex")
> (set (attr "mode")
> - (cond [(and (not (match_test "TARGET_AVX512VL"))
> - (ior (match_operand 0 "ext_sse_reg_operand")
> - (match_operand 1 "ext_sse_reg_operand")))
> - (const_string "XI")
> - (and (eq_attr "alternative" "1")
> + (cond [(and (eq_attr "alternative" "1")
> (match_test "TARGET_AVX512VL"))
> (const_string "OI")
> (ior (match_test "TARGET_SSE_PACKED_SINGLE_INSN_OPTIMAL")
> @@ -1973,7 +1971,8 @@
> {
> if (get_attr_mode (insn) == MODE_V4SF)
> return "%vmovups\t{%1, %0|%0, %1}";
> - else if (get_attr_mode (insn) == MODE_XI)
> + else if (EXT_REX_SSE_REG_P (operands[0])
> + || EXT_REX_SSE_REG_P (operands[1]))
> return "vmovdqu32\t{%1, %0|%0, %1}";
> else
> return "%vmovdqu\t{%1, %0|%0, %1}";
> @@ -1982,7 +1981,8 @@
> {
> if (get_attr_mode (insn) == MODE_V4SF)
> return "%vmovaps\t{%1, %0|%0, %1}";
> - else if (get_attr_mode (insn) == MODE_XI)
> + else if (EXT_REX_SSE_REG_P (operands[0])
> + || EXT_REX_SSE_REG_P (operands[1]))
> return "vmovdqa32\t{%1, %0|%0, %1}";
> else
> return "%vmovdqa\t{%1, %0|%0, %1}";
> @@ -2013,10 +2013,6 @@
> (set (attr "mode")
> (cond [(eq_attr "alternative" "0,1")
> (const_string "DI")
> - (and (not (match_test "TARGET_AVX512VL"))
> - (ior (match_operand 0 "ext_sse_reg_operand")
> - (match_operand 1 "ext_sse_reg_operand")))
> - (const_string "XI")
> (and (eq_attr "alternative" "3")
> (match_test "TARGET_AVX512VL"))
> (const_string "TI")
> diff --git a/gcc/testsuite/gcc.target/i386/pr89229-1.c
> b/gcc/testsuite/gcc.target/i386/pr89229-1.c
> new file mode 100644
> index 00000000000..cce95350bf2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr89229-1.c
> @@ -0,0 +1,47 @@
> +/* { dg-do assemble { target { avx512bw && avx512vl } } } */
> +/* { dg-options "-O1 -mavx512bw -mavx512vl -mtune=skylake-avx512" } */
> +
> +extern void abort (void);
> +extern void exit (int);
> +struct s { unsigned char a[256]; };
> +union u { struct { struct s b; int c; } d; struct { int c; struct s b; } e;
> };
> +static union u v;
> +static union u v0;
> +static struct s *p = &v.d.b;
> +static struct s *q = &v.e.b;
> +
> +static inline struct s rp (void) { return *p; }
> +static inline struct s rq (void) { return *q; }
> +static void pq (void) { *p = rq(); }
> +static void qp (void) { *q = rp(); }
> +
> +static void
> +init (struct s *sp)
> +{
> + int i;
> + for (i = 0; i < 256; i++)
> + sp->a[i] = i;
> +}
> +
> +static void
> +check (struct s *sp)
> +{
> + int i;
> + for (i = 0; i < 256; i++)
> + if (sp->a[i] != i)
> + abort ();
> +}
> +
> +void
> +main_test (void)
> +{
> + v = v0;
> + init (p);
> + qp ();
> + check (q);
> + v = v0;
> + init (q);
> + pq ();
> + check (p);
> + exit (0);
> +}
> --
> 2.20.1
>
