diff mbox

Fix PR rtl-optimization/58295

Message ID 1636116.B9lWmQVdfJ@polaris
State New
Headers show

Commit Message

Eric Botcazou Dec. 9, 2013, 11:18 p.m. UTC
It's the pessimization introduced on the ARM (and other RISC targets) by the 
distribution of truncations in simplify_truncation.  Further information at:
  http://gcc.gnu.org/ml/gcc/2013-12/msg00019.html

The change started as a simple address reassociation trick for x32 and evolved 
into a general distribution of truncations for common operations.  But the 
outcome doesn't really play nice with other transformations that help on RISC 
targets so I think that we should restrict it.  I'm not sure whether it really 
helps on CISC targets either, but it's at least neutral and it's too late to 
backpedal anyway since further changes were installed in the x86 back-end.

Hence the proposed minimal patch based on WORD_REGISTER_OPERATIONS.  Not clear 
whether it's the best long term solution, but I think that we want to fix the 
regression on the 4.8 branch as well.

Tested on x86-64/Linux, PowerPC/Linux and IA-64/Linux.  As reported by Kyrill, 
this fixes gcc.target/arm/unsigned-extend-1.c on the ARM.

Comments?


2013-12-09  Eric Botcazou  <ebotcazou@adacore.com>

	PR rtl-optimization/58295
	* simplify-rtx.c (simplify_truncation): Restrict the distribution for
	WORD_REGISTER_OPERATIONS targets.

Comments

Jeff Law Dec. 10, 2013, 6:35 a.m. UTC | #1
On 12/09/13 16:18, Eric Botcazou wrote:
> It's the pessimization introduced on the ARM (and other RISC targets) by the
> distribution of truncations in simplify_truncation.  Further information at:
>    http://gcc.gnu.org/ml/gcc/2013-12/msg00019.html
>
> The change started as a simple address reassociation trick for x32 and evolved
> into a general distribution of truncations for common operations.  But the
> outcome doesn't really play nice with other transformations that help on RISC
> targets so I think that we should restrict it.  I'm not sure whether it really
> helps on CISC targets either, but it's at least neutral and it's too late to
> backpedal anyway since further changes were installed in the x86 back-end.
>
> Hence the proposed minimal patch based on WORD_REGISTER_OPERATIONS.  Not clear
> whether it's the best long term solution, but I think that we want to fix the
> regression on the 4.8 branch as well.
>
> Tested on x86-64/Linux, PowerPC/Linux and IA-64/Linux.  As reported by Kyrill,
> this fixes gcc.target/arm/unsigned-extend-1.c on the ARM.
>
> Comments?
>
>
> 2013-12-09  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	PR rtl-optimization/58295
> 	* simplify-rtx.c (simplify_truncation): Restrict the distribution for
> 	WORD_REGISTER_OPERATIONS targets.
Seems like a reasonable way to disable until we sort how best to deal 
with this in the long term.

jeff
Kyrylo Tkachov Dec. 10, 2013, 12:25 p.m. UTC | #2
On 09/12/13 23:18, Eric Botcazou wrote:
> It's the pessimization introduced on the ARM (and other RISC targets) by the
> distribution of truncations in simplify_truncation.  Further information at:
>    http://gcc.gnu.org/ml/gcc/2013-12/msg00019.html
>
> The change started as a simple address reassociation trick for x32 and evolved
> into a general distribution of truncations for common operations.  But the
> outcome doesn't really play nice with other transformations that help on RISC
> targets so I think that we should restrict it.  I'm not sure whether it really
> helps on CISC targets either, but it's at least neutral and it's too late to
> backpedal anyway since further changes were installed in the x86 back-end.
>
> Hence the proposed minimal patch based on WORD_REGISTER_OPERATIONS.  Not clear
> whether it's the best long term solution, but I think that we want to fix the
> regression on the 4.8 branch as well.
>
> Tested on x86-64/Linux, PowerPC/Linux and IA-64/Linux.  As reported by Kyrill,
> this fixes gcc.target/arm/unsigned-extend-1.c on the ARM.
>
> Comments?

Hi Eric,

The patch does indeed fix unsigned-extend-1.c on arm and bootstraps succesfully 
on arm-none-linux-gnueabihf.

Thanks for fixing this, that test failure has been spoiling my test runs for 
quite a while now :)

Kyrill

>
>
> 2013-12-09  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	PR rtl-optimization/58295
> 	* simplify-rtx.c (simplify_truncation): Restrict the distribution for
> 	WORD_REGISTER_OPERATIONS targets.
>
>
Eric Botcazou Dec. 10, 2013, 11:02 p.m. UTC | #3
> The patch does indeed fix unsigned-extend-1.c on arm and bootstraps
> succesfully on arm-none-linux-gnueabihf.

Thanks for the testing.

> Thanks for fixing this, that test failure has been spoiling my test runs for
> quite a while now :)

You're welcome.  Now applied on mainline and 4.8 branch, after testing on 
PowerPC/Linux for the latter.
diff mbox

Patch

Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 205774)
+++ simplify-rtx.c	(working copy)
@@ -640,11 +640,16 @@  simplify_truncation (enum machine_mode m
 				   XEXP (op, 0), origmode);
     }
 
-  /* Simplify (truncate:SI (op:DI (x:DI) (y:DI)))
-     to (op:SI (truncate:SI (x:DI)) (truncate:SI (x:DI))).  */
-  if (GET_CODE (op) == PLUS
-      || GET_CODE (op) == MINUS
-      || GET_CODE (op) == MULT)
+  /* If the machine can perform operations in the truncated mode, distribute
+     the truncation, i.e. simplify (truncate:QI (op:SI (x:SI) (y:SI))) into
+     (op:QI (truncate:QI (x:SI)) (truncate:QI (y:SI))).  */
+  if (1
+#ifdef WORD_REGISTER_OPERATIONS
+      && precision >= BITS_PER_WORD
+#endif
+      && (GET_CODE (op) == PLUS
+	  || GET_CODE (op) == MINUS
+	  || GET_CODE (op) == MULT))
     {
       rtx op0 = simplify_gen_unary (TRUNCATE, mode, XEXP (op, 0), op_mode);
       if (op0)