diff mbox

Change replace_rtx if from is a REG (PR target/70245)

Message ID 20160316122257.GD3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek March 16, 2016, 12:22 p.m. UTC 
Hi!

The following testcase is miscompiled on ia32, because
a peephole2 calls replace_rtx trying to replace SImode %ecx with
SImode %edx, but replace_rtx (unlike e.g. simplify_replace_rtx
or validate_replace_rtx), in addition to modifying the rtxes
in place (fine in this case) only does pointer equality check
- if (x == from) return to;, which doesn't work well especially
on hard registers, where they could be different e.g. because
of different ORIGINAL_REGNO, REG_EXPR etc., but still have
the same mode and same REGNO.  Using simplify_replace_rtx
in that peephole2 is possible, but there is a risk that function
simplifies something, turning it from a previously valid address
to invalid one, so it would have to be accomodated with
a validation check and FAIL from the peephole.  Similarly,
validate_replace_rtx needs an instruction, but the peephole2 doesn't have
one yet (and again would need FAIL handling).
If it was just one peephole2 in i386 backend, I'd certainly change just
that, but looking around, at least 2 other targets use replace_rtx
in peephole2s, and quite a few backends use replace_rtx in various places,
so maybe it is better to change replace_rtx to cope with that.

So, this is what we've converged to on IRC and passed bootstrap/regtest
on x86_64-linux and i686-linux.  Is this ok for trunk?

Or is replace_rtx really meant to do pointer equality comparison only and
shall we add another similar function that e.g. supports only hard reg
-> hard reg replacements without simplifications?  What about the case
when the hard reg appears in the expression, but with different mode?
Would that always be the bug of the caller, or shall we do something
different in that case?

2016-03-16  Jakub Jelinek  <jakub@redhat.com>
	    Richard Biener  <rguenth@suse.de>

	PR target/70245
	* rtlanal.c (replace_rtx): For REG, if from is a REG,
	return to even if only REGNO is equal, and assert
	mode is the same.

	* g++.dg/opt/pr70245.C: New test.
	* g++.dg/opt/pr70245.h: New file.
	* g++.dg/opt/pr70245-aux.cc: New file.


	Jakub

Comments

Bernd Schmidt March 16, 2016, 12:59 p.m. UTC | #1 
On 03/16/2016 01:22 PM, Jakub Jelinek wrote:
> So, this is what we've converged to on IRC and passed bootstrap/regtest
> on x86_64-linux and i686-linux.  Is this ok for trunk?

The explanation was a bit confusing at first, but I think this looks 
reasonable. The assert worries me, but triggering it would indicate a 
potential miscompilation, so I think it is preferrable to have it.


Bernd
Segher Boessenkool March 16, 2016, 10:48 p.m. UTC | #2 
On Wed, Mar 16, 2016 at 01:59:29PM +0100, Bernd Schmidt wrote:
> On 03/16/2016 01:22 PM, Jakub Jelinek wrote:
> >So, this is what we've converged to on IRC and passed bootstrap/regtest
> >on x86_64-linux and i686-linux.  Is this ok for trunk?
> 
> The explanation was a bit confusing at first, but I think this looks 
> reasonable. The assert worries me, but triggering it would indicate a 
> potential miscompilation, so I think it is preferrable to have it.

This caused PR70261.  Maybe it's just the assert.


Segher
diff mbox

Patch

--- gcc/rtlanal.c.jj	2016-03-16 10:36:36.000000000 +0100
+++ gcc/rtlanal.c	2016-03-16 10:37:19.317571738 +0100
@@ -2961,7 +2961,16 @@  replace_rtx (rtx x, rtx from, rtx to)
   if (x == 0)
     return 0;
 
-  if (GET_CODE (x) == SUBREG)
+  if (GET_CODE (x) == REG)
+    {
+      if (GET_CODE (from) == REG
+	  && REGNO (x) == REGNO (from))
+	{
+	  gcc_assert (GET_MODE (x) == GET_MODE (from));
+	  return to;
+	}
+    }
+  else if (GET_CODE (x) == SUBREG)
     {
       rtx new_rtx = replace_rtx (SUBREG_REG (x), from, to);
 
--- gcc/testsuite/g++.dg/opt/pr70245.C.jj	2016-03-16 09:02:11.263150597 +0100
+++ gcc/testsuite/g++.dg/opt/pr70245.C	2016-03-16 10:38:02.408955556 +0100
@@ -0,0 +1,52 @@ 
+// PR target/70245
+// { dg-do run }
+// { dg-additional-sources "pr70245-aux.cc" }
+// { dg-options "-O2" }
+// { dg-additional-options "-fPIC" { target fpic } }
+// { dg-additional-options "-march=i386 -mtune=atom" { target ia32 } }
+
+#include "pr70245.h"
+
+struct A *a, *i;
+int b, c, e, l;
+F d;
+
+static A *
+foo (B *x, int *y, int *z)
+{
+  unsigned char *f = (unsigned char *) fn3 (y);
+  D *g = (D *) f;
+  A *h;
+  if (e || a || c || b || g->d)
+    return 0;
+  h = (A *) fn4 ();
+  __builtin_memcpy (h, a, sizeof (A));
+  h->a1 = *(D *) f;
+  if (d)
+    {
+      d (h, x, f + g->d, z);
+      if (*z)
+	fn2 ();
+    }
+  return h;
+}
+
+static A *
+bar (B *x, int *y)
+{
+  int *j = fn1 (x->b, y);
+  if (*y > 0)
+    return 0;
+  i = foo (x, j, y);
+  return i;
+}
+
+B k;
+
+void
+baz (int x)
+{
+  if (x)
+    bar (0, 0);
+  bar (&k, &l);
+}
--- gcc/testsuite/g++.dg/opt/pr70245.h.jj	2016-03-16 09:02:17.073069217 +0100
+++ gcc/testsuite/g++.dg/opt/pr70245.h	2016-03-15 20:19:37.000000000 +0100
@@ -0,0 +1,14 @@ 
+extern struct A *a, *i;
+extern int b, c, e, l;
+int *fn1 (char *, int *);
+void fn2 ();
+void *fn3 (int *);
+struct B { char *b; };
+typedef void (*F) (A *, B *, unsigned char *, int *);
+struct C { int c[16]; };
+struct D { int d; };
+struct A { D a1; C a2; };
+void *fn4 ();
+extern F d;
+extern B k;
+extern void baz (int);
--- gcc/testsuite/g++.dg/opt/pr70245-aux.cc.jj	2016-03-16 09:02:14.113110677 +0100
+++ gcc/testsuite/g++.dg/opt/pr70245-aux.cc	2016-03-16 09:05:04.803719815 +0100
@@ -0,0 +1,56 @@ 
+// PR target/70245
+// { dg-do compile }
+// { dg-options "" }
+
+#include "pr70245.h"
+
+D m;
+A n, o;
+int p, q;
+
+int *
+fn1 (char *x, int *y)
+{
+  *y = 0;
+  return &p;
+}
+
+void
+fn2 ()
+{
+  __builtin_abort ();
+}
+
+void *
+fn3 (int *x)
+{
+  *x = 0;
+  return (void *) &m;
+}
+
+void *
+fn4 ()
+{
+  a = &o;
+  o.a1.d = 9;
+  m.d = sizeof (D);
+  __builtin_memcpy (o.a2.c, "abcdefghijklmnop", 16);
+  return (void *) &n;
+}
+
+void
+fn5 (A *x, B *y, unsigned char *z, int *w)
+{
+  if (x != &n || y != &k || z != (unsigned char *) (&m + 1))
+    __builtin_abort ();
+  q++;
+}
+
+int
+main ()
+{
+  d = fn5;
+  baz (0);
+  if (q != 1)
+    __builtin_abort ();
+}