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

2016-01-12 Thread Marcus Shawcroft 
On 18 December 2015 at 12:13, James Greenhalgh  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  
>
> Backport from mainline.
> 2015-12-09  James Greenhalgh  
>
> 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_low): Use
> zero_extract rather than truncate.
> (aarch64_movdi_high): Likewise.
>
> gcc/testsuite/
>
> 2015-12-14  James Greenhalgh  
>
> Backport from mainline.
> 2015-12-09  James Greenhalgh  
>
> PR rtl-optimization/67609
> * gcc.dg/torture/pr67609.c: New.
>

OK /Marcus

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

2016-01-11 Thread James Greenhalgh 
On Fri, Dec 18, 2015 at 12:13:31PM +, James Greenhalgh wrote:
> 
> On Mon, Dec 14, 2015 at 11:49:26AM +, Marcus Shawcroft wrote:
> > On 14 December 2015 at 11:01, James Greenhalgh  
> > wrote:
> > > On Wed, Dec 09, 2015 at 01:13:20PM +, Marcus Shawcroft wrote:
> > >> On 27 November 2015 at 13:01, James Greenhalgh 
> > >>  wrote:
> > >>
> > >> > 2015-11-27  James Greenhalgh  
> > >> >
> > >> > * 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_low): Use
> > >> > zero_extract rather than truncate.
> > >> > (aarch64_movdi_high): Likewise.
> > >> >
> > >> > 2015-11-27  James Greenhalgh  
> > >> >
> > >> > * 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  
> 
>   Backport from mainline.
>   2015-12-09  James Greenhalgh  
> 
>   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_low): Use
>   zero_extract rather than truncate.
>   (aarch64_movdi_high): Likewise.
> 
> gcc/testsuite/
> 
> 2015-12-14  James Greenhalgh  
> 
>   Backport from mainline.
>   2015-12-09  James Greenhalgh  
> 
>   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_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_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")
> +

Backport: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

2015-12-18 Thread James Greenhalgh 

On Mon, Dec 14, 2015 at 11:49:26AM +, Marcus Shawcroft wrote:
> On 14 December 2015 at 11:01, James Greenhalgh  
> wrote:
> > On Wed, Dec 09, 2015 at 01:13:20PM +, Marcus Shawcroft wrote:
> >> On 27 November 2015 at 13:01, James Greenhalgh  
> >> wrote:
> >>
> >> > 2015-11-27  James Greenhalgh  
> >> >
> >> > * 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_low): Use
> >> > zero_extract rather than truncate.
> >> > (aarch64_movdi_high): Likewise.
> >> >
> >> > 2015-11-27  James Greenhalgh  
> >> >
> >> > * 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  

Backport from mainline.
2015-12-09  James Greenhalgh  

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_low): Use
zero_extract rather than truncate.
(aarch64_movdi_high): Likewise.

gcc/testsuite/

2015-12-14  James Greenhalgh  

Backport from mainline.
2015-12-09  James Greenhalgh  

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_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_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 000..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;
+

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

2015-12-14 Thread Marcus Shawcroft 
On 14 December 2015 at 11:01, James Greenhalgh  wrote:
> On Wed, Dec 09, 2015 at 01:13:20PM +, Marcus Shawcroft wrote:
>> On 27 November 2015 at 13:01, James Greenhalgh  
>> wrote:
>>
>> > 2015-11-27  James Greenhalgh  
>> >
>> > * 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_low): Use
>> > zero_extract rather than truncate.
>> > (aarch64_movdi_high): Likewise.
>> >
>> > 2015-11-27  James Greenhalgh  
>> >
>> > * 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

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

2015-12-14 Thread James Greenhalgh 
On Wed, Dec 09, 2015 at 01:13:20PM +, Marcus Shawcroft wrote:
> On 27 November 2015 at 13:01, James Greenhalgh  
> wrote:
> 
> > 2015-11-27  James Greenhalgh  
> >
> > * 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_low): Use
> > zero_extract rather than truncate.
> > (aarch64_movdi_high): Likewise.
> >
> > 2015-11-27  James Greenhalgh  
> >
> > * 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

2015-12-09 Thread James Greenhalgh 
On Fri, Nov 27, 2015 at 01:01:01PM +, 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 [0x1fff]))
> 
> The simplify routines want to turn this around to look like:
> 
>   (truncate:DI (and:TF (reg:TF 32 v0 [ a ])
>   (const_int 2305843009213693951 [0x1fff])))
> 
> 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  
> 
>   * 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_low): Use
>   zero_extract rather than truncate.
>   (aarch64_movdi_high): Likewise.
> 
> 2015-11-27  James Greenhalgh  
> 
>   * 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

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

2015-12-09 Thread Marcus Shawcroft 
On 27 November 2015 at 13:01, James Greenhalgh  wrote:

> 2015-11-27  James Greenhalgh  
>
> * 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_low): Use
> zero_extract rather than truncate.
> (aarch64_movdi_high): Likewise.
>
> 2015-11-27  James Greenhalgh  
>
> * gcc.dg/torture/pr67609.c: New.
>

+ detailed dicussion.  In all other cases, we want to be premissive

s/premissive/permissive/

OK /Marcus

[Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

2015-11-27 Thread James Greenhalgh 

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 [0x1fff]))

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

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

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  

* 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_low): Use
zero_extract rather than truncate.
(aarch64_movdi_high): Likewise.

2015-11-27  James Greenhalgh  

* 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)
+

Re: [Patch AArch64] Reinstate CANNOT_CHANGE_MODE_CLASS to fix pr67609

2015-11-27 Thread Richard Biener 
On Fri, Nov 27, 2015 at 2:01 PM, James Greenhalgh
 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 [0x1fff]))
>
> The simplify routines want to turn this around to look like:
>
>   (truncate:DI (and:TF (reg:TF 32 v0 [ a ])
>   (const_int 2305843009213693951 [0x1fff])))
>
> 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  
>
> * 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_low): Use
> zero_extract rather than truncate.
> (aarch64_movdi_high): Likewise.
>
> 2015-11-27  James Greenhalgh  
>
> * gcc.dg/torture/pr67609.c: New.
>