diff mbox

[PR55315] Don't assume a nonzero address plus a const is a nonzero address

Message ID 50A766EA.7000507@mentor.com
State New
Headers show

Commit Message

Tom de Vries Nov. 17, 2012, 10:28 a.m. UTC
Richard,

This patch fixes PR 55315.

When compiling for the mips target with -O2, function f is folded to 0, while
the address of data is not known compile-time:
...
int data[4096];

int
f (void)
{
  return (((unsigned int) &data[0]) == 0xdeadbea0U);
}
....

What happens is that expand turns the comparison into the equivalent of:
...
  (((unsigned int) &data[0]) + (~0xdeadbea0U + 1)) == 0
...
and then nonzero_address_p triggers here during cse:
...
    case PLUS:
      if (CONST_INT_P (XEXP (x, 1)))
        return nonzero_address_p (XEXP (x, 0));
...
and '(((unsigned int) &data[0]) + (~0xdeadbea0U + 1))' is considered a nonzero
address, and the comparison evaluates to 0.

This example is related to PR 29519, in fact you mention the code pattern expand
generates in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29519#c5.
But the patch for that PR does not address this example.

This patch prevents nonzero_address_p from assuming that a nonzero address plus
a const is a nonzero address.

Build and reg-tested on mips.

OK for trunk?

Thanks,
- Tom

2012-11-17  Tom de Vries  <tom@codesourcery.com>

	PR rtl-optimization/55315

	* rtlanal.c (nonzero_address_p): Don't assume a nonzero address plus a
	const is a nonzero address.

	* gcc.target/mips/pr55315.c: New test.

Comments

Richard Sandiford Nov. 18, 2012, 10:50 a.m. UTC | #1
Tom de Vries <Tom_deVries@mentor.com> writes:
> 2012-11-17  Tom de Vries  <tom@codesourcery.com>
>
> 	PR rtl-optimization/55315
>
> 	* rtlanal.c (nonzero_address_p): Don't assume a nonzero address plus a
> 	const is a nonzero address.
>
> 	* gcc.target/mips/pr55315.c: New test.

OK, thanks.

Richard
Jakub Jelinek Nov. 19, 2012, 1 p.m. UTC | #2
On Sun, Nov 18, 2012 at 10:50:53AM +0000, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
> > 2012-11-17  Tom de Vries  <tom@codesourcery.com>
> >
> > 	PR rtl-optimization/55315
> >
> > 	* rtlanal.c (nonzero_address_p): Don't assume a nonzero address plus a
> > 	const is a nonzero address.
> >
> > 	* gcc.target/mips/pr55315.c: New test.
> 
> OK, thanks.

I think this is just papering over the problem that we don't distinguish
between pointer and integer arithmetics at RTL level (yeah, there is
REG_POINTER, but can we take it as guaranteed that integer arithmetics
is performed without REG_POINTER while pointer arithmetics with them?),
but in many places in RTL we just assume pointer arithmetics rules.
The above assumption that nonzero_address_p plus constant is nonzero
should be fine assumption in pointer arithmetics, so are many aliasing
assumptions, but they aren't valid for integer arithmetics.  We have several
open PRs for this I believe.

	Jakub
Tom de Vries Nov. 19, 2012, 3:10 p.m. UTC | #3
On 19/11/12 14:00, Jakub Jelinek wrote:
> On Sun, Nov 18, 2012 at 10:50:53AM +0000, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>> 2012-11-17  Tom de Vries  <tom@codesourcery.com>
>>>
>>> 	PR rtl-optimization/55315
>>>
>>> 	* rtlanal.c (nonzero_address_p): Don't assume a nonzero address plus a
>>> 	const is a nonzero address.
>>>
>>> 	* gcc.target/mips/pr55315.c: New test.
>>
>> OK, thanks.
> 

Jakub,

> I think this is just papering over the problem that we don't distinguish
> between pointer and integer arithmetics at RTL level

I'm not sure this is a proper description, since the patch fixes a problem in
the compiler where it generates incorrect code while the test-case is valid.
I agree we can fix this more optimally if we can accurately distinguish between
pointer and integer arithmetics at RTL level.
And as I've just learned from PR 49330, the patch doesn't fix all places where
we incorrectly assume pointer arithmetic, but it does fix one of them.

> (yeah, there is
> REG_POINTER, but can we take it as guaranteed that integer arithmetics
> is performed without REG_POINTER while pointer arithmetics with them?),
> but in many places in RTL we just assume pointer arithmetics rules.
> The above assumption that nonzero_address_p plus constant is nonzero
> should be fine assumption in pointer arithmetics, so are many aliasing
> assumptions, but they aren't valid for integer arithmetics.  We have several
> open PRs for this I believe.

Thanks for making me aware of that. I've found PR 49330 and duplicate PR 52517.
PR 49330 comment 1 lists an example similar to this PR (but AFAIU the
cse/nonzero_address_p scenario is not explicitly discussed there).

Thanks,
- Tom

> 
> 	Jakub
>
diff mbox

Patch

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c (revision 193478)
+++ gcc/rtlanal.c (working copy)
@@ -392,10 +392,8 @@  nonzero_address_p (const_rtx x)
       return nonzero_address_p (XEXP (x, 0));
 
     case PLUS:
-      if (CONST_INT_P (XEXP (x, 1)))
-        return nonzero_address_p (XEXP (x, 0));
       /* Handle PIC references.  */
-      else if (XEXP (x, 0) == pic_offset_table_rtx
+      if (XEXP (x, 0) == pic_offset_table_rtx
 	       && CONSTANT_P (XEXP (x, 1)))
 	return true;
       return false;
Index: gcc/testsuite/gcc.target/mips/pr55315.c
===================================================================
--- /dev/null (new file)
+++ gcc/testsuite/gcc.target/mips/pr55315.c (revision 0)
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+
+int data[4096];
+
+int
+f (void)
+{
+  return (((unsigned int) &data[0]) == 0xdeadbea0U);
+}
+
+/* { dg-final { scan-assembler-not "\tmove\t\\\$2,\\\$0" } } */