[PATCH] i386: Use EXT_REX_SSE_REG_P in *movoi_internal_avx/movti_internal
H.J. Lu
hjl.tools@gmail.com
Mon Feb 11 13:29:00 GMT 2019
On Mon, Feb 11, 2019 at 5:15 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Mon, Feb 11, 2019 at 2:10 PM H.J. Lu <hjl.tools@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.
There is
switch (get_attr_type (insn))
{
case TYPE_SSELOG1:
return standard_sse_constant_opcode (insn, operands);
standard_sse_constant_opcode has
else if (x == constm1_rtx || vector_all_ones_operand (x, mode))
{
enum attr_mode insn_mode = get_attr_mode (insn);
switch (insn_mode)
{
case MODE_XI:
case MODE_V8DF:
case MODE_V16SF:
gcc_assert (TARGET_AVX512F);
return "vpternlogd\t{$0xFF, %g0, %g0, %g0|%g0, %g0, %g0, 0xFF}";
What mode should be used to set %xmm23 to -1 with AVX512VL? What mode
should be used to load %xmm23 with AVX512VL? There is no need to
check ext_sse_reg_operand here the same as in
(define_insn "mov<mode>_internal"
[(set (match_operand:VMOVE 0 "nonimmediate_operand"
"=v,v ,v ,m")
(match_operand:VMOVE 1 "nonimmediate_or_sse_const_operand"
" C,BC,vm,v"))]
"TARGET_SSE
&& (register_operand (operands[0], <MODE>mode)
|| register_operand (operands[1], <MODE>mode))"
{
> 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.lu@intel.com>
> > Jakub Jelinek <jakub@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
> >
--
H.J.
More information about the Gcc-patches
mailing list