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? 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