diff mbox

[RFA,pr,target/60648] Fix non-canonical RTL from x86 backend -- P1 regression

Message ID 5335B990.2020600@redhat.com
State New
Headers show

Commit Message

Jeff Law March 28, 2014, 6:04 p.m. UTC 
On 03/26/14 12:28, Jakub Jelinek wrote:
> On Wed, Mar 26, 2014 at 12:17:43PM -0600, Jeff Law wrote:
>> On 03/26/14 12:12, Jakub Jelinek wrote:
>>> On Wed, Mar 26, 2014 at 11:02:48AM -0600, Jeff Law wrote:
>>>> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.
>>>> Verified it fixes the original and reduced testcase.
>>>
>>> Note, the testcase is missing from your patch.
>>>
>>> But I'd question if this is the right place to canonicalize it.
>>> The non-canonical order seems to be created in the generic code, where
>>> do_tablejump does:
>> No, at that point it's still canonical because the x86 backend
>> hasn't simpified the (mult ...) subexpression.  Its the
>> simplification of that subexpression to a constant that creates the
>> non-canonical RTL.  That's why I fixed the x86 bits -- those are the
>> bits that simplify the (mult ...) into a (const_int) and thus
>> creates the non-canonical RTL.
>
> (mult:SI (const_int 0) (const_int 4)) is IMHO far from being canonical.
> And, I'd say it is likely other target legitimization hooks would also try
> to simplify it similarly.
> simplify_gen_binary is used in several other places during expansion,
> so I don't see why it couldn't be desirable here.

Here's the updated patch.  It uses simplify_gen_binary in expr.c to 
simplify the address expression as we're building it.  It also uses 
copy_addr_to_reg in the x86 backend to avoid the possibility of 
generating non-canonical RTL there too.

By accident I interrupted the regression test cycle, so that is still 
running.  OK for the trunk if that passes?

Comments

Jakub Jelinek March 28, 2014, 6:12 p.m. UTC | #1 
On Fri, Mar 28, 2014 at 12:04:00PM -0600, Jeff Law wrote:
> Here's the updated patch.  It uses simplify_gen_binary in expr.c to
> simplify the address expression as we're building it.  It also uses
> copy_addr_to_reg in the x86 backend to avoid the possibility of
> generating non-canonical RTL there too.
> 
> By accident I interrupted the regression test cycle, so that is
> still running.  OK for the trunk if that passes?

Ok, thanks.

	Jakub
Andreas Schwab March 29, 2014, 1 p.m. UTC | #2 
Jeff Law <law@redhat.com> writes:

> diff --git a/gcc/testsuite/g++.dg/pr60648.C b/gcc/testsuite/g++.dg/pr60648.C
> new file mode 100644
> index 0000000..80c0561
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr60648.C
> @@ -0,0 +1,73 @@
> +/* { dg-do compile } */
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O3 -fPIC -m32" }  */

xg++: error: unrecognized command line option '-m32'

Andreas.
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 53d58b3..3caae44 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,12 @@ 
+2014-03-27  Jeff Law  <law@redhat.com>
+	    Jakub Jalinek <jakub@redhat.com>
+
+       * expr.c (do_tablejump): Use simplify_gen_binary rather than
+       gen_rtx_{PLUS,MULT} to build up the address expression.
+
+       * i386/i386.c (ix86_legitimize_address): Use copy_addr_to_reg to avoid
+       creating non-canonical RTL.
+
 2014-03-26  Richard Biener  <rguenther@suse.de>
 
 	* tree-pretty-print.c (percent_K_format): Implement special
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 842be68..70b8f02 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -13925,13 +13925,13 @@  ix86_legitimize_address (rtx x, rtx oldx ATTRIBUTE_UNUSED,
       if (GET_CODE (XEXP (x, 0)) == MULT)
 	{
 	  changed = 1;
-	  XEXP (x, 0) = force_operand (XEXP (x, 0), 0);
+	  XEXP (x, 0) = copy_addr_to_reg (XEXP (x, 0));
 	}
 
       if (GET_CODE (XEXP (x, 1)) == MULT)
 	{
 	  changed = 1;
-	  XEXP (x, 1) = force_operand (XEXP (x, 1), 0);
+	  XEXP (x, 1) = copy_addr_to_reg (XEXP (x, 1));
 	}
 
       if (changed
diff --git a/gcc/expr.c b/gcc/expr.c
index cdb4551..ebf136e 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -11134,11 +11134,12 @@  do_tablejump (rtx index, enum machine_mode mode, rtx range, rtx table_label,
      GET_MODE_SIZE, because this indicates how large insns are.  The other
      uses should all be Pmode, because they are addresses.  This code
      could fail if addresses and insns are not the same size.  */
-  index = gen_rtx_PLUS
-    (Pmode,
-     gen_rtx_MULT (Pmode, index,
-		   gen_int_mode (GET_MODE_SIZE (CASE_VECTOR_MODE), Pmode)),
-     gen_rtx_LABEL_REF (Pmode, table_label));
+  index = simplify_gen_binary (MULT, Pmode, index,
+			       gen_int_mode (GET_MODE_SIZE (CASE_VECTOR_MODE),
+					     Pmode));
+  index = simplify_gen_binary (PLUS, Pmode, index,
+			       gen_rtx_LABEL_REF (Pmode, table_label));
+
 #ifdef PIC_CASE_VECTOR_ADDRESS
   if (flag_pic)
     index = PIC_CASE_VECTOR_ADDRESS (index);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index cdc8e9a..fc3c198 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2014-03-27  Jeff Law  <law@redhat.com>
+
+       PR target/60648
+       * g++.dg/pr60648.C: New test.
+
 2014-03-26  Jakub Jelinek  <jakub@redhat.com>
 
 	PR sanitizer/60636
diff --git a/gcc/testsuite/g++.dg/pr60648.C b/gcc/testsuite/g++.dg/pr60648.C
new file mode 100644
index 0000000..80c0561
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr60648.C
@@ -0,0 +1,73 @@ 
+/* { dg-do compile } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O3 -fPIC -m32" }  */
+
+enum component
+{
+  Ex,
+  Ez,
+  Hy,
+  Permeability
+};
+enum derived_component
+{};
+enum direction
+{
+  X,
+  Y,
+  Z,
+  R,
+  P,
+  NO_DIRECTION
+};
+derived_component a;
+component *b;
+component c;
+direction d;
+inline direction fn1 (component p1)
+{
+  switch (p1)
+    {
+    case 0:
+      return Y;
+    case 1:
+      return Z;
+    case Permeability:
+      return NO_DIRECTION;
+    }
+  return X;
+}
+
+inline component fn2 (direction p1)
+{
+  switch (p1)
+    {
+    case 0:
+    case 1:
+      return component ();
+    case Z:
+    case R:
+      return component (1);
+    case P:
+      return component (3);
+    }
+}
+
+void fn3 ()
+{
+  direction e;
+  switch (0)
+  case 0:
+  switch (a)
+    {
+    case 0:
+      c = Ex;
+      b[1] = Hy;
+    }
+  e = fn1 (b[1]);
+  b[1] = fn2 (e);
+  d = fn1 (c);
+}
+
+
+