Fix PR 67609

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

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

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

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

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~