| Message ID | 1450440811-2928-1-git-send-email-james.greenhalgh@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 18, 2015 at 12:13:31PM +0000, James Greenhalgh wrote: > > On Mon, Dec 14, 2015 at 11:49:26AM +0000, Marcus Shawcroft wrote: > > On 14 December 2015 at 11:01, James Greenhalgh <james.greenhalgh@arm.com> wrote: > > > On Wed, Dec 09, 2015 at 01:13:20PM +0000, Marcus Shawcroft wrote: > > >> On 27 November 2015 at 13:01, James Greenhalgh <james.greenhalgh@arm.com> wrote: > > >> > > >> > 2015-11-27 James Greenhalgh <james.greenhalgh@arm.com> > > >> > > > >> > * config/aarch64/aarch64-protos.h > > >> > (aarch64_cannot_change_mode_class): Bring back. > > >> > * config/aarch64/aarch64.c > > >> > (aarch64_cannot_change_mode_class): Likewise. > > >> > * config/aarch64/aarch64.h (CANNOT_CHANGE_MODE_CLASS): Likewise. > > >> > * config/aarch64/aarch64.md (aarch64_movdi_<mode>low): Use > > >> > zero_extract rather than truncate. > > >> > (aarch64_movdi_<mode>high): Likewise. > > >> > > > >> > 2015-11-27 James Greenhalgh <james.greenhalgh@arm.com> > > >> > > > >> > * gcc.dg/torture/pr67609.c: New. > > >> > > > >> > > >> + detailed dicussion. In all other cases, we want to be premissive > > >> > > >> s/premissive/permissive/ > > >> > > >> OK /Marcus > > > > > > Thanks. > > > > > > This has had a week or so to soak on trunk now, is it OK to backport to GCC > > > 5 and 4.9? > > > > > > The patch applies as-good-as clean, with only a little bit to fix up in > > > aarch64-protos.h to keep alphabetical order, and I've bootstrapped and tested > > > the backports with no issue. > > > > OK /Marcus > > > > Looking back at the patch just before I hit commit, the 4.9 backport was > a little different (as we still have a CANNOT_CHANGE_MODE_CLASS there). > We can drop the aarch64-protos.h and aarch64.h changes, and we need to > change the sense of the new check, such that we can return true for the > case added by this patch, and false for the limited number of other safe > cases in 4.9. *ping* Thanks, James > > Bootstrapped on aarch64-none-linux-gnu. > > OK? > > Thanks, > James > > --- > gcc/ > > 2015-12-14 James Greenhalgh <james.greenhalgh@arm.com> > > Backport from mainline. > 2015-12-09 James Greenhalgh <james.greenhalgh@arm.com> > > PR rtl-optimization/67609 > * config/aarch64/aarch64.c > (aarch64_cannot_change_mode_class): Don't permit word_mode > subregs of full vector modes. > * config/aarch64/aarch64.md (aarch64_movdi_<mode>low): Use > zero_extract rather than truncate. > (aarch64_movdi_<mode>high): Likewise. > > gcc/testsuite/ > > 2015-12-14 James Greenhalgh <james.greenhalgh@arm.com> > > Backport from mainline. > 2015-12-09 James Greenhalgh <james.greenhalgh@arm.com> > > PR rtl-optimization/67609 > * gcc.dg/torture/pr67609.c: New. > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 8153997..5ca38b6 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -8405,6 +8405,18 @@ aarch64_cannot_change_mode_class (enum machine_mode from, > enum machine_mode to, > enum reg_class rclass) > { > + /* We cannot allow word_mode subregs of full vector modes. > + Otherwise the middle-end will assume it's ok to store to > + (subreg:DI (reg:TI 100) 0) in order to modify only the low 64 bits > + of the 128-bit register. However, after reload the subreg will > + be dropped leaving a plain DImode store. See PR67609 for a more > + detailed dicussion. In some other cases we can be permissive and > + return false. */ > + if (reg_classes_intersect_p (FP_REGS, rclass) > + && GET_MODE_SIZE (to) == UNITS_PER_WORD > + && GET_MODE_SIZE (from) > UNITS_PER_WORD) > + return true; > + > /* Full-reg subregs are allowed on general regs or any class if they are > the same size. */ > if (GET_MODE_SIZE (from) == GET_MODE_SIZE (to) > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 24bb029..d6c6b1e 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -3454,7 +3454,8 @@ > > (define_insn "aarch64_movdi_<mode>low" > [(set (match_operand:DI 0 "register_operand" "=r") > - (truncate:DI (match_operand:TX 1 "register_operand" "w")))] > + (zero_extract:DI (match_operand:TX 1 "register_operand" "w") > + (const_int 64) (const_int 0)))] > "reload_completed || reload_in_progress" > "fmov\\t%x0, %d1" > [(set_attr "type" "f_mrc") > @@ -3463,9 +3464,8 @@ > > (define_insn "aarch64_movdi_<mode>high" > [(set (match_operand:DI 0 "register_operand" "=r") > - (truncate:DI > - (lshiftrt:TX (match_operand:TX 1 "register_operand" "w") > - (const_int 64))))] > + (zero_extract:DI (match_operand:TX 1 "register_operand" "w") > + (const_int 64) (const_int 64)))] > "reload_completed || reload_in_progress" > "fmov\\t%x0, %1.d[1]" > [(set_attr "type" "f_mrc") > diff --git a/gcc/testsuite/gcc.dg/torture/pr67609.c b/gcc/testsuite/gcc.dg/torture/pr67609.c > new file mode 100644 > index 0000000..817857d > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/torture/pr67609.c > @@ -0,0 +1,33 @@ > +/* { dg-do run } */ > + > +typedef union > +{ > + double v[2]; > + double s __attribute__ ((vector_size (16))); > +} data; > + > +data reg; > + > +void __attribute__ ((noinline)) > +set_lower (double b) > +{ > + data stack_var; > + double __attribute__ ((vector_size (16))) one = { 1.0, 1.0 }; > + stack_var.s = reg.s; > + stack_var.s += one; > + stack_var.v[0] += b; > + reg.s = stack_var.s; > +} > + > +int > +main (int argc, char ** argv) > +{ > + reg.v[0] = 1.0; > + reg.v[1] = 1.0; > + /* reg should contain { 1.0, 1.0 }. */ > + set_lower (2.0); > + /* reg should contain { 4.0, 2.0 }. */ > + if ((int) reg.v[0] != 4 || (int) reg.v[1] != 2) > + __builtin_abort (); > + return 0; > +}
On 18 December 2015 at 12:13, James Greenhalgh <james.greenhalgh@arm.com> wrote: > Looking back at the patch just before I hit commit, the 4.9 backport was > a little different (as we still have a CANNOT_CHANGE_MODE_CLASS there). > We can drop the aarch64-protos.h and aarch64.h changes, and we need to > change the sense of the new check, such that we can return true for the > case added by this patch, and false for the limited number of other safe > cases in 4.9. > > Bootstrapped on aarch64-none-linux-gnu. > > OK? > > Thanks, > James > > --- > gcc/ > > 2015-12-14 James Greenhalgh <james.greenhalgh@arm.com> > > Backport from mainline. > 2015-12-09 James Greenhalgh <james.greenhalgh@arm.com> > > PR rtl-optimization/67609 > * config/aarch64/aarch64.c > (aarch64_cannot_change_mode_class): Don't permit word_mode > subregs of full vector modes. > * config/aarch64/aarch64.md (aarch64_movdi_<mode>low): Use > zero_extract rather than truncate. > (aarch64_movdi_<mode>high): Likewise. > > gcc/testsuite/ > > 2015-12-14 James Greenhalgh <james.greenhalgh@arm.com> > > Backport from mainline. > 2015-12-09 James Greenhalgh <james.greenhalgh@arm.com> > > PR rtl-optimization/67609 > * gcc.dg/torture/pr67609.c: New. > OK /Marcus
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 8153997..5ca38b6 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -8405,6 +8405,18 @@ aarch64_cannot_change_mode_class (enum machine_mode from, enum machine_mode to, enum reg_class rclass) { + /* We cannot allow word_mode subregs of full vector modes. + Otherwise the middle-end will assume it's ok to store to + (subreg:DI (reg:TI 100) 0) in order to modify only the low 64 bits + of the 128-bit register. However, after reload the subreg will + be dropped leaving a plain DImode store. See PR67609 for a more + detailed dicussion. In some other cases we can be permissive and + return false. */ + if (reg_classes_intersect_p (FP_REGS, rclass) + && GET_MODE_SIZE (to) == UNITS_PER_WORD + && GET_MODE_SIZE (from) > UNITS_PER_WORD) + return true; + /* Full-reg subregs are allowed on general regs or any class if they are the same size. */ if (GET_MODE_SIZE (from) == GET_MODE_SIZE (to) diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 24bb029..d6c6b1e 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -3454,7 +3454,8 @@ (define_insn "aarch64_movdi_<mode>low" [(set (match_operand:DI 0 "register_operand" "=r") - (truncate:DI (match_operand:TX 1 "register_operand" "w")))] + (zero_extract:DI (match_operand:TX 1 "register_operand" "w") + (const_int 64) (const_int 0)))] "reload_completed || reload_in_progress" "fmov\\t%x0, %d1" [(set_attr "type" "f_mrc") @@ -3463,9 +3464,8 @@ (define_insn "aarch64_movdi_<mode>high" [(set (match_operand:DI 0 "register_operand" "=r") - (truncate:DI - (lshiftrt:TX (match_operand:TX 1 "register_operand" "w") - (const_int 64))))] + (zero_extract:DI (match_operand:TX 1 "register_operand" "w") + (const_int 64) (const_int 64)))] "reload_completed || reload_in_progress" "fmov\\t%x0, %1.d[1]" [(set_attr "type" "f_mrc") diff --git a/gcc/testsuite/gcc.dg/torture/pr67609.c b/gcc/testsuite/gcc.dg/torture/pr67609.c new file mode 100644 index 0000000..817857d --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr67609.c @@ -0,0 +1,33 @@ +/* { dg-do run } */ + +typedef union +{ + double v[2]; + double s __attribute__ ((vector_size (16))); +} data; + +data reg; + +void __attribute__ ((noinline)) +set_lower (double b) +{ + data stack_var; + double __attribute__ ((vector_size (16))) one = { 1.0, 1.0 }; + stack_var.s = reg.s; + stack_var.s += one; + stack_var.v[0] += b; + reg.s = stack_var.s; +} + +int +main (int argc, char ** argv) +{ + reg.v[0] = 1.0; + reg.v[1] = 1.0; + /* reg should contain { 1.0, 1.0 }. */ + set_lower (2.0); + /* reg should contain { 4.0, 2.0 }. */ + if ((int) reg.v[0] != 4 || (int) reg.v[1] != 2) + __builtin_abort (); + return 0; +}