Fix PR 67609

Fix PR 67609

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

The problem in this PR is that a word-mode subreg is used to write to a
multi-word pseudo, under the assumption that is the correct way to insert a
value into the appropriate bits of the pseudo.

Except that the pseudo then gets assigned to an SSE register, at which point
all of the assumptions start to fall apart.  Primarily, (subreg X 0) and
(subreg X 8) do not in the end resolve to different hard registers, so an
assignment to (subreg X 0) may legitimately clobber all of X.

There *are* ways to insert a value into an element of an SSE register, but what
comes out of reload is indistinguishable from a normal DImode assignment.

An ideal solution would be to use a different method than subregs for
multi-word registers.  Using the same code for "view convert", insert, and
extract is going to continue to cause us problems.

I had a look over the other major vector targets:

  * ppc, sparc, s390 already disallow the described condition.

  * arm ought not be subject to this problem because each vector register is
composed of 4 individually addressable registers.  So when (subreg X N) is
simplified, we really do resolve to the desired hard register.

  * aarch64 is almost certainly vulnerable, since it deleted its
CANNOT_CHANGE_MODE_CLASS implementation in January.  I haven't tried to create
a test case that fails for it, but I'm certain it's possible.


Tested on x86_64 and committed.


r~

[-- Attachment #2: zz --]
[-- Type: text/plain, Size: 5783 bytes --]

	PR rtl-opt/67609
	* config/i386/i386.c (ix86_cannot_change_mode_class): Disallow
	narrowing subregs on SSE and MMX registers.
	* doc/tm.texi.in (CANNOT_CHANGE_MODE_CLASS): Clarify when subregs that
	appear to be sub-words of multi-register pseudos must be rejected.
	* doc/tm.texi: Regenerate.
testsuite/
	* gcc.target/i386/pr67609-2.c: New test.

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index d05c8f8..82fd054 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -43031,15 +43031,22 @@ ix86_cannot_change_mode_class (machine_mode from, machine_mode to,
   if (MAYBE_FLOAT_CLASS_P (regclass))
     return true;
 
+  /* Vector registers do not support QI or HImode loads.  If we don't
+     disallow a change to these modes, reload will assume it's ok to
+     drop the subreg from (subreg:SI (reg:HI 100) 0).  This affects
+     the vec_dupv4hi pattern.
+
+     Further, 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.  This is indistinguishable
+     from a "normal" DImode move, and so we're justified to use movsd,
+     which modifies the entire 128-bit register.
+
+     Combining these two conditions, disallow all narrowing mode changes.  */
   if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))
-    {
-      /* Vector registers do not support QI or HImode loads.  If we don't
-	 disallow a change to these modes, reload will assume it's ok to
-	 drop the subreg from (subreg:SI (reg:HI 100) 0).  This affects
-	 the vec_dupv4hi pattern.  */
-      if (GET_MODE_SIZE (from) < 4)
-	return true;
-    }
+    return GET_MODE_SIZE (to) < GET_MODE_SIZE (from);
 
   return false;
 }
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index a8666b1..606ddb6 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -2823,8 +2823,8 @@ in the reload pass.
 If defined, a C expression that returns nonzero for a @var{class} for which
 a change from mode @var{from} to mode @var{to} is invalid.
 
-For the example, loading 32-bit integer or floating-point objects into
-floating-point registers on the Alpha extends them to 64 bits.
+For example, loading 32-bit integer or floating-point objects into
+floating-point registers on Alpha extends them to 64 bits.
 Therefore loading a 64-bit object and then storing it as a 32-bit object
 does not store the low-order 32 bits, as would be the case for a normal
 register.  Therefore, @file{alpha.h} defines @code{CANNOT_CHANGE_MODE_CLASS}
@@ -2835,6 +2835,17 @@ as below:
   (GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO) \
    ? reg_classes_intersect_p (FLOAT_REGS, (CLASS)) : 0)
 @end smallexample
+
+Even if storing from a register in mode @var{to} would be valid,
+if both @var{from} and @code{raw_reg_mode} for @var{class} are wider
+than @code{word_mode}, then we must prevent @var{to} narrowing the
+mode.  This happens when the middle-end assumes that it can load
+or store pieces of an @var{N}-word pseudo, and that the pseudo will
+eventually be allocated to @var{N} @code{word_mode} hard registers.
+Failure to prevent this kind of mode change will result in the
+entire @code{raw_reg_mode} being modified instead of the partial
+value that the middle-end intended.
+
 @end defmac
 
 @deftypefn {Target Hook} reg_class_t TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS (int, @var{reg_class_t})
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 69b6cf9..93620eb 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -2461,8 +2461,8 @@ in the reload pass.
 If defined, a C expression that returns nonzero for a @var{class} for which
 a change from mode @var{from} to mode @var{to} is invalid.
 
-For the example, loading 32-bit integer or floating-point objects into
-floating-point registers on the Alpha extends them to 64 bits.
+For example, loading 32-bit integer or floating-point objects into
+floating-point registers on Alpha extends them to 64 bits.
 Therefore loading a 64-bit object and then storing it as a 32-bit object
 does not store the low-order 32 bits, as would be the case for a normal
 register.  Therefore, @file{alpha.h} defines @code{CANNOT_CHANGE_MODE_CLASS}
@@ -2473,6 +2473,17 @@ as below:
   (GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO) \
    ? reg_classes_intersect_p (FLOAT_REGS, (CLASS)) : 0)
 @end smallexample
+
+Even if storing from a register in mode @var{to} would be valid,
+if both @var{from} and @code{raw_reg_mode} for @var{class} are wider
+than @code{word_mode}, then we must prevent @var{to} narrowing the
+mode.  This happens when the middle-end assumes that it can load
+or store pieces of an @var{N}-word pseudo, and that the pseudo will
+eventually be allocated to @var{N} @code{word_mode} hard registers.
+Failure to prevent this kind of mode change will result in the
+entire @code{raw_reg_mode} being modified instead of the partial
+value that the middle-end intended.
+
 @end defmac
 
 @hook TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
diff --git a/gcc/testsuite/gcc.target/i386/pr67609-2.c b/gcc/testsuite/gcc.target/i386/pr67609-2.c
new file mode 100644
index 0000000..fece437
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr67609-2.c
@@ -0,0 +1,29 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -msse2" } */
+/* { dg-require-effective-target sse2 } */
+
+#include <stdlib.h>
+#include <emmintrin.h>
+
+__m128d reg = { 2.0, 4.0 };
+
+void
+__attribute__((noinline))
+set_lower (double b)
+{
+  double v[2];
+  _mm_store_pd(v, reg);
+  v[0] = b;
+  reg = _mm_load_pd(v);
+}
+
+int
+main ()
+{
+  set_lower (6.0);
+
+  if (reg[1] != 4.0)
+    abort ();
+
+  return 0;
+}

Re: Fix PR 67609

[H.J. Lu]

On Tue, Oct 27, 2015 at 1:21 PM, Richard Henderson <rth@redhat.com> wrote:
> The problem in this PR is that a word-mode subreg is used to write to a
> multi-word pseudo, under the assumption that is the correct way to insert a
> value into the appropriate bits of the pseudo.
>
> Except that the pseudo then gets assigned to an SSE register, at which point
> all of the assumptions start to fall apart.  Primarily, (subreg X 0) and
> (subreg X 8) do not in the end resolve to different hard registers, so an
> assignment to (subreg X 0) may legitimately clobber all of X.
>
> There *are* ways to insert a value into an element of an SSE register, but what
> comes out of reload is indistinguishable from a normal DImode assignment.
>
> An ideal solution would be to use a different method than subregs for
> multi-word registers.  Using the same code for "view convert", insert, and
> extract is going to continue to cause us problems.
>
> I had a look over the other major vector targets:
>
>   * ppc, sparc, s390 already disallow the described condition.
>
>   * arm ought not be subject to this problem because each vector register is
> composed of 4 individually addressable registers.  So when (subreg X N) is
> simplified, we really do resolve to the desired hard register.
>
>   * aarch64 is almost certainly vulnerable, since it deleted its
> CANNOT_CHANGE_MODE_CLASS implementation in January.  I haven't tried to create
> a test case that fails for it, but I'm certain it's possible.
>
>
> Tested on x86_64 and committed.
>

It caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68112


-- 
H.J.

Re: Fix PR 67609

[H.J. Lu]

On Tue, Oct 27, 2015 at 4:45 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Oct 27, 2015 at 1:21 PM, Richard Henderson <rth@redhat.com> wrote:
>> The problem in this PR is that a word-mode subreg is used to write to a
>> multi-word pseudo, under the assumption that is the correct way to insert a
>> value into the appropriate bits of the pseudo.
>>
>> Except that the pseudo then gets assigned to an SSE register, at which point
>> all of the assumptions start to fall apart.  Primarily, (subreg X 0) and
>> (subreg X 8) do not in the end resolve to different hard registers, so an
>> assignment to (subreg X 0) may legitimately clobber all of X.
>>
>> There *are* ways to insert a value into an element of an SSE register, but what
>> comes out of reload is indistinguishable from a normal DImode assignment.
>>
>> An ideal solution would be to use a different method than subregs for
>> multi-word registers.  Using the same code for "view convert", insert, and
>> extract is going to continue to cause us problems.
>>
>> I had a look over the other major vector targets:
>>
>>   * ppc, sparc, s390 already disallow the described condition.
>>
>>   * arm ought not be subject to this problem because each vector register is
>> composed of 4 individually addressable registers.  So when (subreg X N) is
>> simplified, we really do resolve to the desired hard register.
>>
>>   * aarch64 is almost certainly vulnerable, since it deleted its
>> CANNOT_CHANGE_MODE_CLASS implementation in January.  I haven't tried to create
>> a test case that fails for it, but I'm certain it's possible.
>>
>>
>> Tested on x86_64 and committed.
>>
>
> It caused:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68112

Oops.  It should be:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68124


-- 
H.J.

Re: Fix PR 67609

[James Greenhalgh]

On Tue, Oct 27, 2015 at 01:21:51PM -0700, Richard Henderson wrote:
> The problem in this PR is that a word-mode subreg is used to write to a
> multi-word pseudo, under the assumption that is the correct way to insert a
> value into the appropriate bits of the pseudo.
> 
> Except that the pseudo then gets assigned to an SSE register, at which point
> all of the assumptions start to fall apart.  Primarily, (subreg X 0) and
> (subreg X 8) do not in the end resolve to different hard registers, so an
> assignment to (subreg X 0) may legitimately clobber all of X.
> 
> There *are* ways to insert a value into an element of an SSE register, but what
> comes out of reload is indistinguishable from a normal DImode assignment.
> 
> An ideal solution would be to use a different method than subregs for
> multi-word registers.  Using the same code for "view convert", insert, and
> extract is going to continue to cause us problems.
> 
> I had a look over the other major vector targets:
> 
>   * ppc, sparc, s390 already disallow the described condition.
> 
>   * arm ought not be subject to this problem because each vector register is
> composed of 4 individually addressable registers.  So when (subreg X N) is
> simplified, we really do resolve to the desired hard register.
> 
>   * aarch64 is almost certainly vulnerable, since it deleted its
> CANNOT_CHANGE_MODE_CLASS implementation in January.  I haven't tried to create
> a test case that fails for it, but I'm certain it's possible.

The best I've come up with so far needs some union-hackery that I'm not
convinced is legal, and looks like:

  typedef union
  {
    double v[2];
    double s __attribute__ ((vector_size (16)));
  } data;

  data reg;

  void
  set_lower (double b)
  {
    dodgy_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;
  }

This shows the issue going back to GCC 4.9, the code we generate for
AArch64 looks like:

set_lower:
	fmov	v2.2d, 1.0e+0
	adrp	x0, reg
	ldr	q1, [x0, #:lo12:reg]
	fadd	v1.2d, v1.2d, v2.2d
	orr	v2.16b, v1.16b, v1.16b
	fadd	d2, d0, d1 //               <----- Clobbered stack_var.v[1].
	str	q2, [x0, #:lo12:reg] //     <----- Wrote zeroes to the top half of reg
	ret

Reading the documentation you add below, we'd need to bring back
CANNOT_CHANGE_MODE_CLASS for AArch64 and forbid changes from wide registers
to 64-bit (and larger) values. Is this right?  Are these workarounds intended
to be temporary, or is the midend bug likely to be fixed? It isn't clear
from the PR and the documentation you add how long-lived the workaround will
be, and it is a shame to lose the otherwise useful optimisations enabled by a
permissive CANNOT_CHANGE_MODE_CLASS.

Thanks,
James

> 
> 
> Tested on x86_64 and committed.
> 
> 
> r~

> 	PR rtl-opt/67609
> 	* config/i386/i386.c (ix86_cannot_change_mode_class): Disallow
> 	narrowing subregs on SSE and MMX registers.
> 	* doc/tm.texi.in (CANNOT_CHANGE_MODE_CLASS): Clarify when subregs that
> 	appear to be sub-words of multi-register pseudos must be rejected.
> 	* doc/tm.texi: Regenerate.
> testsuite/
> 	* gcc.target/i386/pr67609-2.c: New test.
> 
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index d05c8f8..82fd054 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -43031,15 +43031,22 @@ ix86_cannot_change_mode_class (machine_mode from, machine_mode to,
>    if (MAYBE_FLOAT_CLASS_P (regclass))
>      return true;
>  
> +  /* Vector registers do not support QI or HImode loads.  If we don't
> +     disallow a change to these modes, reload will assume it's ok to
> +     drop the subreg from (subreg:SI (reg:HI 100) 0).  This affects
> +     the vec_dupv4hi pattern.
> +
> +     Further, 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.  This is indistinguishable
> +     from a "normal" DImode move, and so we're justified to use movsd,
> +     which modifies the entire 128-bit register.
> +
> +     Combining these two conditions, disallow all narrowing mode changes.  */
>    if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))
> -    {
> -      /* Vector registers do not support QI or HImode loads.  If we don't
> -	 disallow a change to these modes, reload will assume it's ok to
> -	 drop the subreg from (subreg:SI (reg:HI 100) 0).  This affects
> -	 the vec_dupv4hi pattern.  */
> -      if (GET_MODE_SIZE (from) < 4)
> -	return true;
> -    }
> +    return GET_MODE_SIZE (to) < GET_MODE_SIZE (from);
>  
>    return false;
>  }
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index a8666b1..606ddb6 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -2823,8 +2823,8 @@ in the reload pass.
>  If defined, a C expression that returns nonzero for a @var{class} for which
>  a change from mode @var{from} to mode @var{to} is invalid.
>  
> -For the example, loading 32-bit integer or floating-point objects into
> -floating-point registers on the Alpha extends them to 64 bits.
> +For example, loading 32-bit integer or floating-point objects into
> +floating-point registers on Alpha extends them to 64 bits.
>  Therefore loading a 64-bit object and then storing it as a 32-bit object
>  does not store the low-order 32 bits, as would be the case for a normal
>  register.  Therefore, @file{alpha.h} defines @code{CANNOT_CHANGE_MODE_CLASS}
> @@ -2835,6 +2835,17 @@ as below:
>    (GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO) \
>     ? reg_classes_intersect_p (FLOAT_REGS, (CLASS)) : 0)
>  @end smallexample
> +
> +Even if storing from a register in mode @var{to} would be valid,
> +if both @var{from} and @code{raw_reg_mode} for @var{class} are wider
> +than @code{word_mode}, then we must prevent @var{to} narrowing the
> +mode.  This happens when the middle-end assumes that it can load
> +or store pieces of an @var{N}-word pseudo, and that the pseudo will
> +eventually be allocated to @var{N} @code{word_mode} hard registers.
> +Failure to prevent this kind of mode change will result in the
> +entire @code{raw_reg_mode} being modified instead of the partial
> +value that the middle-end intended.
> +
>  @end defmac
>  
>  @deftypefn {Target Hook} reg_class_t TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS (int, @var{reg_class_t})
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 69b6cf9..93620eb 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -2461,8 +2461,8 @@ in the reload pass.
>  If defined, a C expression that returns nonzero for a @var{class} for which
>  a change from mode @var{from} to mode @var{to} is invalid.
>  
> -For the example, loading 32-bit integer or floating-point objects into
> -floating-point registers on the Alpha extends them to 64 bits.
> +For example, loading 32-bit integer or floating-point objects into
> +floating-point registers on Alpha extends them to 64 bits.
>  Therefore loading a 64-bit object and then storing it as a 32-bit object
>  does not store the low-order 32 bits, as would be the case for a normal
>  register.  Therefore, @file{alpha.h} defines @code{CANNOT_CHANGE_MODE_CLASS}
> @@ -2473,6 +2473,17 @@ as below:
>    (GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO) \
>     ? reg_classes_intersect_p (FLOAT_REGS, (CLASS)) : 0)
>  @end smallexample
> +
> +Even if storing from a register in mode @var{to} would be valid,
> +if both @var{from} and @code{raw_reg_mode} for @var{class} are wider
> +than @code{word_mode}, then we must prevent @var{to} narrowing the
> +mode.  This happens when the middle-end assumes that it can load
> +or store pieces of an @var{N}-word pseudo, and that the pseudo will
> +eventually be allocated to @var{N} @code{word_mode} hard registers.
> +Failure to prevent this kind of mode change will result in the
> +entire @code{raw_reg_mode} being modified instead of the partial
> +value that the middle-end intended.
> +
>  @end defmac
>  
>  @hook TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
> diff --git a/gcc/testsuite/gcc.target/i386/pr67609-2.c b/gcc/testsuite/gcc.target/i386/pr67609-2.c
> new file mode 100644
> index 0000000..fece437
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67609-2.c
> @@ -0,0 +1,29 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -msse2" } */
> +/* { dg-require-effective-target sse2 } */
> +
> +#include <stdlib.h>
> +#include <emmintrin.h>
> +
> +__m128d reg = { 2.0, 4.0 };
> +
> +void
> +__attribute__((noinline))
> +set_lower (double b)
> +{
> +  double v[2];
> +  _mm_store_pd(v, reg);
> +  v[0] = b;
> +  reg = _mm_load_pd(v);
> +}
> +
> +int
> +main ()
> +{
> +  set_lower (6.0);
> +
> +  if (reg[1] != 4.0)
> +    abort ();
> +
> +  return 0;
> +}

Re: Fix PR 67609

[Richard Henderson]

On 11/25/2015 06:14 PM, James Greenhalgh wrote:
> On Tue, Oct 27, 2015 at 01:21:51PM -0700, Richard Henderson wrote:
>>    * aarch64 is almost certainly vulnerable, since it deleted its
>> CANNOT_CHANGE_MODE_CLASS implementation in January.  I haven't tried to create
>> a test case that fails for it, but I'm certain it's possible.
>
> The best I've come up with so far needs some union-hackery that I'm not
> convinced is legal, and looks like:
>
>    typedef union
>    {
>      double v[2];
>      double s __attribute__ ((vector_size (16)));
>    } data;
>
>    data reg;
>
>    void
>    set_lower (double b)
>    {
>      dodgy_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;
>    }

Modulo the one typo, this is a valid test case.

> This shows the issue going back to GCC 4.9, the code we generate for
> AArch64 looks like:
>
> set_lower:
> 	fmov	v2.2d, 1.0e+0
> 	adrp	x0, reg
> 	ldr	q1, [x0, #:lo12:reg]
> 	fadd	v1.2d, v1.2d, v2.2d
> 	orr	v2.16b, v1.16b, v1.16b
> 	fadd	d2, d0, d1 //               <----- Clobbered stack_var.v[1].
> 	str	q2, [x0, #:lo12:reg] //     <----- Wrote zeroes to the top half of reg
> 	ret

And yes, we mis-compile it, for the same reason:

(insn 13 12 14 2 (set (subreg:DF (reg/v:TI 78 [ stack_var ]) 0)
         (reg:DF 85)) /localhome/devel/rth/z.c:16 54 {*movdf_aarch64}
      (nil))

becomes

(insn 13 11 16 2 (set (subreg:DF (reg/v:TI 78 [ stack_var ]) 0)
         (plus:DF (reg:DF 32 v0 [ b ])
             (subreg:DF (reg:V2DF 82) 0))) /localhome/devel/rth/z.c:16 804 {adddf3}
      (expr_list:REG_DEAD (reg:V2DF 82)
         (expr_list:REG_DEAD (reg:DF 32 v0 [ b ])
             (nil))))

becomes

(insn:TI 13 11 17 (set (reg:DF 34 v2 [orig:78 stack_var ] [78])
         (plus:DF (reg:DF 32 v0 [ b ])
             (reg:DF 33 v1 [82]))) /localhome/devel/rth/z.c:16 804 {adddf3}
      (expr_list:REG_DEAD (reg:DF 33 v1 [82])
         (expr_list:REG_DEAD (reg:DF 32 v0 [ b ])
             (nil))))

> Reading the documentation you add below, we'd need to bring back
> CANNOT_CHANGE_MODE_CLASS for AArch64 and forbid changes from wide registers
> to 64-bit (and larger) values. Is this right?

Not exactly -- forbid BITS_PER_WORD (64-bit) subregs of hard registers > 
BITS_PER_WORD.  See the verbiage I added to the i386 backend for this.


>  Are these workarounds intended
> to be temporary, or is the midend bug likely to be fixed?

Not in the near term, no.  We'd need to replace subreg, which that does 3 jobs 
simultaneously, with something else that's less ambiguous.


r~

[Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]


Hi,

This patch follow Richard Henderson's advice to tighten up
CANNOT_CHANGE_MODE_CLASS for AArch64 to avoid a simplification bug in
the middle-end.

There is nothing AArch64-specific about the testcase which triggers this,
so I'll put it in the testcase for other targets. If you see a regression,
the explanation in the PR is much more thorough and correct than I can
reproduce here, so I'd recommend starting there. In short, target
maintainers need to:

> forbid BITS_PER_WORD (64-bit) subregs of hard registers >
> BITS_PER_WORD.  See the verbiage I added to the i386 backend for this.

We removed the CANNOT_CHANGE_MODE_CLASS macro back in January 2015. Before
then, we used it to workaround bugs in big-endian vector support
( https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01216.html ). Ideally,
we'd not need to bring this macro back, but if we can't fix the middle-end
bug this exposes, we need the workaround.

For AArch64, doing this runs in to some trouble with two of our
instruction patterns - we end up with:

  (truncate:DI (reg:TF))

Which fails if it ever make it through to the simplify routines with
something nasty like:

  (and:DI (truncate:DI (reg:TF 32 v0 [ a ]))
      (const_int 2305843009213693951 [0x1fffffffffffffff]))

The simplify routines want to turn this around to look like:

  (truncate:DI (and:TF (reg:TF 32 v0 [ a ])
      (const_int 2305843009213693951 [0x1fffffffffffffff])))

Which then wants to further simplify the expression by first building
the constant in TF mode, and trunc_int_for_mode barfs:

  0x7a38a5 trunc_int_for_mode(long, machine_mode)
	  .../gcc/explow.c:53

We can fix that by changing the patterns to use a zero_extract, which seems
more in line with what they actually express (extracting the two 64-bit
halves of a 128-bit value).

Bootstrapped on aarch64-none-linux-gnu, and tested on aarch64-none-elf and
aarch64_be-none-elf without seeing any correctness regressions.

OK?

If so, we ought to get this backported to the release branches, the gcc-5
backport applies clean (testing ongoing but looks OK so far) if the release
managers and AArch64 maintainers agree this is something that should be
backported this late in the 5.3 release cycle.

Thanks,
James

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-AArch64-Reinstate-CANNOT_CHANGE_MODE_CLASS-to-.patch --]
[-- Type: text/x-patch;  name=0001-Patch-AArch64-Reinstate-CANNOT_CHANGE_MODE_CLASS-to-.patch, Size: 4315 bytes --]

diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index e0a050c..59d3da4 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -269,6 +269,9 @@ int aarch64_get_condition_code (rtx);
 bool aarch64_bitmask_imm (HOST_WIDE_INT val, machine_mode);
 int aarch64_branch_cost (bool, bool);
 enum aarch64_symbol_type aarch64_classify_symbolic_expression (rtx);
+bool aarch64_cannot_change_mode_class (machine_mode,
+				       machine_mode,
+				       enum reg_class);
 bool aarch64_const_vec_all_same_int_p (rtx, HOST_WIDE_INT);
 bool aarch64_constant_address_p (rtx);
 bool aarch64_expand_movmem (rtx *);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 3fe2f0f..fadb716 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -12408,6 +12408,24 @@ aarch64_vectorize_vec_perm_const_ok (machine_mode vmode,
   return ret;
 }
 
+/* Implement target hook CANNOT_CHANGE_MODE_CLASS.  */
+bool
+aarch64_cannot_change_mode_class (machine_mode from,
+				  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 all other cases, we want to be premissive
+     and return false.  */
+  return (reg_classes_intersect_p (FP_REGS, rclass)
+	  && GET_MODE_SIZE (to) == UNITS_PER_WORD
+	  && GET_MODE_SIZE (from) > UNITS_PER_WORD);
+}
+
 rtx
 aarch64_reverse_mask (enum machine_mode mode)
 {
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 68c006f..66b768d 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -831,6 +831,9 @@ do {									     \
   extern void  __aarch64_sync_cache_range (void *, void *);	\
   __aarch64_sync_cache_range (beg, end)
 
+#define CANNOT_CHANGE_MODE_CLASS(FROM, TO, CLASS)	\
+  aarch64_cannot_change_mode_class (FROM, TO, CLASS)
+
 #define SHIFT_COUNT_TRUNCATED !TARGET_SIMD
 
 /* Choose appropriate mode for caller saves, so we do the minimum
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 64a40ae..c3367df 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4667,7 +4667,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)))]
   "TARGET_FLOAT && (reload_completed || reload_in_progress)"
   "fmov\\t%x0, %d1"
   [(set_attr "type" "f_mrc")
@@ -4676,9 +4677,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)))]
   "TARGET_FLOAT && (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;
+}

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[Richard Biener]

On Fri, Nov 27, 2015 at 2:01 PM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> This patch follow Richard Henderson's advice to tighten up
> CANNOT_CHANGE_MODE_CLASS for AArch64 to avoid a simplification bug in
> the middle-end.
>
> There is nothing AArch64-specific about the testcase which triggers this,
> so I'll put it in the testcase for other targets. If you see a regression,
> the explanation in the PR is much more thorough and correct than I can
> reproduce here, so I'd recommend starting there. In short, target
> maintainers need to:
>
>> forbid BITS_PER_WORD (64-bit) subregs of hard registers >
>> BITS_PER_WORD.  See the verbiage I added to the i386 backend for this.
>
> We removed the CANNOT_CHANGE_MODE_CLASS macro back in January 2015. Before
> then, we used it to workaround bugs in big-endian vector support
> ( https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01216.html ). Ideally,
> we'd not need to bring this macro back, but if we can't fix the middle-end
> bug this exposes, we need the workaround.
>
> For AArch64, doing this runs in to some trouble with two of our
> instruction patterns - we end up with:
>
>   (truncate:DI (reg:TF))
>
> Which fails if it ever make it through to the simplify routines with
> something nasty like:
>
>   (and:DI (truncate:DI (reg:TF 32 v0 [ a ]))
>       (const_int 2305843009213693951 [0x1fffffffffffffff]))
>
> The simplify routines want to turn this around to look like:
>
>   (truncate:DI (and:TF (reg:TF 32 v0 [ a ])
>       (const_int 2305843009213693951 [0x1fffffffffffffff])))
>
> Which then wants to further simplify the expression by first building
> the constant in TF mode, and trunc_int_for_mode barfs:
>
>   0x7a38a5 trunc_int_for_mode(long, machine_mode)
>           .../gcc/explow.c:53
>
> We can fix that by changing the patterns to use a zero_extract, which seems
> more in line with what they actually express (extracting the two 64-bit
> halves of a 128-bit value).
>
> Bootstrapped on aarch64-none-linux-gnu, and tested on aarch64-none-elf and
> aarch64_be-none-elf without seeing any correctness regressions.
>
> OK?
>
> If so, we ought to get this backported to the release branches, the gcc-5
> backport applies clean (testing ongoing but looks OK so far) if the release
> managers and AArch64 maintainers agree this is something that should be
> backported this late in the 5.3 release cycle.

Your call, the RC will be done on monday.

Richard.

> Thanks,
> James
>
> ---
> 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.
>

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[James Greenhalgh]

On Fri, Nov 27, 2015 at 01:01:01PM +0000, James Greenhalgh wrote:
> This patch follow Richard Henderson's advice to tighten up
> CANNOT_CHANGE_MODE_CLASS for AArch64 to avoid a simplification bug in
> the middle-end.
> 
> There is nothing AArch64-specific about the testcase which triggers this,
> so I'll put it in the testcase for other targets. If you see a regression,
> the explanation in the PR is much more thorough and correct than I can
> reproduce here, so I'd recommend starting there. In short, target
> maintainers need to:
> 
> > forbid BITS_PER_WORD (64-bit) subregs of hard registers >
> > BITS_PER_WORD.  See the verbiage I added to the i386 backend for this.
> 
> We removed the CANNOT_CHANGE_MODE_CLASS macro back in January 2015. Before
> then, we used it to workaround bugs in big-endian vector support
> ( https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01216.html ). Ideally,
> we'd not need to bring this macro back, but if we can't fix the middle-end
> bug this exposes, we need the workaround.
> 
> For AArch64, doing this runs in to some trouble with two of our
> instruction patterns - we end up with:
> 
>   (truncate:DI (reg:TF))
> 
> Which fails if it ever make it through to the simplify routines with
> something nasty like:
> 
>   (and:DI (truncate:DI (reg:TF 32 v0 [ a ]))
>       (const_int 2305843009213693951 [0x1fffffffffffffff]))
> 
> The simplify routines want to turn this around to look like:
> 
>   (truncate:DI (and:TF (reg:TF 32 v0 [ a ])
>       (const_int 2305843009213693951 [0x1fffffffffffffff])))
> 
> Which then wants to further simplify the expression by first building
> the constant in TF mode, and trunc_int_for_mode barfs:
> 
>   0x7a38a5 trunc_int_for_mode(long, machine_mode)
> 	  .../gcc/explow.c:53
> 
> We can fix that by changing the patterns to use a zero_extract, which seems
> more in line with what they actually express (extracting the two 64-bit
> halves of a 128-bit value).
> 
> Bootstrapped on aarch64-none-linux-gnu, and tested on aarch64-none-elf and
> aarch64_be-none-elf without seeing any correctness regressions.
> 
> OK?

*ping*

Thanks,
James


> 
> If so, we ought to get this backported to the release branches, the gcc-5
> backport applies clean (testing ongoing but looks OK so far) if the release
> managers and AArch64 maintainers agree this is something that should be
> backported this late in the 5.3 release cycle.
> 
> Thanks,
> James
> 
> ---
> 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.
> 

> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index e0a050c..59d3da4 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -269,6 +269,9 @@ int aarch64_get_condition_code (rtx);
>  bool aarch64_bitmask_imm (HOST_WIDE_INT val, machine_mode);
>  int aarch64_branch_cost (bool, bool);
>  enum aarch64_symbol_type aarch64_classify_symbolic_expression (rtx);
> +bool aarch64_cannot_change_mode_class (machine_mode,
> +				       machine_mode,
> +				       enum reg_class);
>  bool aarch64_const_vec_all_same_int_p (rtx, HOST_WIDE_INT);
>  bool aarch64_constant_address_p (rtx);
>  bool aarch64_expand_movmem (rtx *);
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 3fe2f0f..fadb716 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -12408,6 +12408,24 @@ aarch64_vectorize_vec_perm_const_ok (machine_mode vmode,
>    return ret;
>  }
>  
> +/* Implement target hook CANNOT_CHANGE_MODE_CLASS.  */
> +bool
> +aarch64_cannot_change_mode_class (machine_mode from,
> +				  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 all other cases, we want to be premissive
> +     and return false.  */
> +  return (reg_classes_intersect_p (FP_REGS, rclass)
> +	  && GET_MODE_SIZE (to) == UNITS_PER_WORD
> +	  && GET_MODE_SIZE (from) > UNITS_PER_WORD);
> +}
> +
>  rtx
>  aarch64_reverse_mask (enum machine_mode mode)
>  {
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 68c006f..66b768d 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -831,6 +831,9 @@ do {									     \
>    extern void  __aarch64_sync_cache_range (void *, void *);	\
>    __aarch64_sync_cache_range (beg, end)
>  
> +#define CANNOT_CHANGE_MODE_CLASS(FROM, TO, CLASS)	\
> +  aarch64_cannot_change_mode_class (FROM, TO, CLASS)
> +
>  #define SHIFT_COUNT_TRUNCATED !TARGET_SIMD
>  
>  /* Choose appropriate mode for caller saves, so we do the minimum
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 64a40ae..c3367df 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -4667,7 +4667,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)))]
>    "TARGET_FLOAT && (reload_completed || reload_in_progress)"
>    "fmov\\t%x0, %d1"
>    [(set_attr "type" "f_mrc")
> @@ -4676,9 +4677,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)))]
>    "TARGET_FLOAT && (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;
> +}

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[Marcus Shawcroft]

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

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[James Greenhalgh]

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.

Cheers,
James

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[Marcus Shawcroft]

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

Backport: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Backport-Patch-AArch64-Reinstate-CANNOT_CHANGE_MODE_.patch --]
[-- Type: text/x-patch;  name=0001-Backport-Patch-AArch64-Reinstate-CANNOT_CHANGE_MODE_.patch, Size: 3004 bytes --]

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

Re: Backport: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[James Greenhalgh]

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

Re: Backport: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

[Marcus Shawcroft]

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