diff mbox

Backport: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

Message ID 1450440811-2928-1-git-send-email-james.greenhalgh@arm.com
State New
Headers show

Commit Message

James Greenhalgh Dec. 18, 2015, 12:13 p.m. UTC
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.

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.

Comments

James Greenhalgh Jan. 11, 2016, 12:12 p.m. UTC | #1
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;

> +}
Marcus Shawcroft Jan. 12, 2016, 10:32 a.m. UTC | #2
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 mbox

Patch

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;
+}