PATCH: bug in find_reloads_address_part

PATCH: bug in find_reloads_address_part

[Sandra Loosemore]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

One of the new MIPS features I'm working on tripped over an obvious bug in 
reload.c.  This patch fixes the particular problem I ran into, but could someone 
who's familiar with this code look and see if the first branch of the if (where 
X is a constant and not a PLUS containing a constant) is also incorrect in 
passing &tem instead of &x?  It looks suspicious to me.

-Sandra


[-- Attachment #2: reload.log --]
[-- Type: text/x-log, Size: 165 bytes --]

2007-07-19  Sandra Loosemore  <sandra@codesourcery.com>

	gcc/
	* reload.c (find_reloads_address_part):  Pass correct MEMREFLOC
	argument to find_reloads_address.



[-- Attachment #3: reload.patch --]
[-- Type: text/x-patch, Size: 744 bytes --]

Index: gcc/reload.c
===================================================================
*** gcc/reload.c	(revision 126701)
--- gcc/reload.c	(working copy)
*************** find_reloads_address_part (rtx x, rtx *l
*** 5940,5946 ****
  
        tem = force_const_mem (GET_MODE (x), XEXP (x, 1));
        x = gen_rtx_PLUS (GET_MODE (x), XEXP (x, 0), tem);
!       find_reloads_address (mode, &tem, XEXP (tem, 0), &XEXP (tem, 0),
  			    opnum, type, ind_levels, 0);
      }
  
--- 5940,5946 ----
  
        tem = force_const_mem (GET_MODE (x), XEXP (x, 1));
        x = gen_rtx_PLUS (GET_MODE (x), XEXP (x, 0), tem);
!       find_reloads_address (mode, &XEXP (x, 1), XEXP (tem, 0), &XEXP (tem, 0),
  			    opnum, type, ind_levels, 0);
      }
  

Re: PATCH: bug in find_reloads_address_part

[Ian Lance Taylor]

Sandra Loosemore <sandra@codesourcery.com> writes:

> One of the new MIPS features I'm working on tripped over an obvious
> bug in reload.c.  This patch fixes the particular problem I ran into,
> but could someone who's familiar with this code look and see if the
> first branch of the if (where X is a constant and not a PLUS
> containing a constant) is also incorrect in passing &tem instead of
> &x?  It looks suspicious to me.
> 
> -Sandra
> 
> 
> 2007-07-19  Sandra Loosemore  <sandra@codesourcery.com>
> 
> 	gcc/
> 	* reload.c (find_reloads_address_part):  Pass correct MEMREFLOC
> 	argument to find_reloads_address.

I have to agree that this appears to fix an obvious bug.  However,
it's rather unnerving that this bug appears to have existed since the
beginning of time.

I think the code in the first part of find_reloads_address_part should
almost certainly pass loc rathern than &tem in the call to
find_reloads_address.

I'll approve your patch and that patch if they pass bootstrap and
testing on a primary platform.

Thanks.

Ian

Re: PATCH: bug in find_reloads_address_part

[Richard Sandiford]

Ian Lance Taylor <iant@google.com> writes:
> Sandra Loosemore <sandra@codesourcery.com> writes:
>> One of the new MIPS features I'm working on tripped over an obvious
>> bug in reload.c.  This patch fixes the particular problem I ran into,
>> but could someone who's familiar with this code look and see if the
>> first branch of the if (where X is a constant and not a PLUS
>> containing a constant) is also incorrect in passing &tem instead of
>> &x?  It looks suspicious to me.
>> 
>> -Sandra
>> 
>> 
>> 2007-07-19  Sandra Loosemore  <sandra@codesourcery.com>
>> 
>> 	gcc/
>> 	* reload.c (find_reloads_address_part):  Pass correct MEMREFLOC
>> 	argument to find_reloads_address.
>
> I have to agree that this appears to fix an obvious bug.  However,
> it's rather unnerving that this bug appears to have existed since the
> beginning of time.
>
> I think the code in the first part of find_reloads_address_part should
> almost certainly pass loc rathern than &tem in the call to
> find_reloads_address.

I thought that too when I first saw it, because I assumed that the
second argument would be stored.  But as far as I can tell, that isn't
true; the pointer never escapes.  It would be wrong to pass loc rather
than "&tem" because loc is not yet a memory reference, whereas "*memrefloc"
must be.

I'd have thought the argument ought to be "&x" rather than "&tem" though.
It's "x" that we pass to push_reload, and we want to pass the copied
memory rather than the original, since it's the copy that gets the
reloads.  I don't really see why the first part needs to use "tem" at all.
(It's inefficient to copy the MEM in this situation, because force_const_mem
already returns a unique MEM.  That's a different issue though.)

Richard

Re: PATCH: bug in find_reloads_address_part

[Sandra Loosemore]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

Richard Sandiford wrote:
> Ian Lance Taylor <iant@google.com> writes:
>> I think the code in the first part of find_reloads_address_part should
>> almost certainly pass loc rathern than &tem in the call to
>> find_reloads_address.
> 
> I thought that too when I first saw it, because I assumed that the
> second argument would be stored.  But as far as I can tell, that isn't
> true; the pointer never escapes.  It would be wrong to pass loc rather
> than "&tem" because loc is not yet a memory reference, whereas "*memrefloc"
> must be.
> 
> I'd have thought the argument ought to be "&x" rather than "&tem" though.
> It's "x" that we pass to push_reload, and we want to pass the copied
> memory rather than the original, since it's the copy that gets the
> reloads.  I don't really see why the first part needs to use "tem" at all.
> (It's inefficient to copy the MEM in this situation, because force_const_mem
> already returns a unique MEM.  That's a different issue though.)

Richard's understanding of this issue matches mine.  FWIW, here's the version of 
the patch I'm currently testing.  Assuming this passes, is it OK to commit?

-Sandra

[-- Attachment #2: reload1.patch --]
[-- Type: text/x-patch, Size: 1387 bytes --]

Index: gcc/reload.c
===================================================================
*** gcc/reload.c	(revision 127051)
--- gcc/reload.c	(working copy)
*************** find_reloads_address_part (rtx x, rtx *l
*** 5923,5932 ****
        && (! LEGITIMATE_CONSTANT_P (x)
  	  || PREFERRED_RELOAD_CLASS (x, class) == NO_REGS))
      {
!       rtx tem;
! 
!       tem = x = force_const_mem (mode, x);
!       find_reloads_address (mode, &tem, XEXP (tem, 0), &XEXP (tem, 0),
  			    opnum, type, ind_levels, 0);
      }
  
--- 5923,5930 ----
        && (! LEGITIMATE_CONSTANT_P (x)
  	  || PREFERRED_RELOAD_CLASS (x, class) == NO_REGS))
      {
!       x = force_const_mem (mode, x);
!       find_reloads_address (mode, &x, XEXP (x, 0), &XEXP (x, 0),
  			    opnum, type, ind_levels, 0);
      }
  
*************** find_reloads_address_part (rtx x, rtx *l
*** 5939,5945 ****
  
        tem = force_const_mem (GET_MODE (x), XEXP (x, 1));
        x = gen_rtx_PLUS (GET_MODE (x), XEXP (x, 0), tem);
!       find_reloads_address (mode, &tem, XEXP (tem, 0), &XEXP (tem, 0),
  			    opnum, type, ind_levels, 0);
      }
  
--- 5937,5943 ----
  
        tem = force_const_mem (GET_MODE (x), XEXP (x, 1));
        x = gen_rtx_PLUS (GET_MODE (x), XEXP (x, 0), tem);
!       find_reloads_address (mode, &XEXP (x, 1), XEXP (tem, 0), &XEXP (tem, 0),
  			    opnum, type, ind_levels, 0);
      }
  

Re: PATCH: bug in find_reloads_address_part

[Ian Lance Taylor]

Richard Sandiford <richard@codesourcery.com> writes:

> I thought that too when I first saw it, because I assumed that the
> second argument would be stored.  But as far as I can tell, that isn't
> true; the pointer never escapes.  It would be wrong to pass loc rather
> than "&tem" because loc is not yet a memory reference, whereas "*memrefloc"
> must be.

Ah, yes, you're right.

Sandra Loosemore <sandra@codesourcery.com> writes:

> Richard's understanding of this issue matches mine.  FWIW, here's the
> version of the patch I'm currently testing.  Assuming this passes, is
> it OK to commit?

Yes.

Thanks.

Ian

Re: PATCH: bug in find_reloads_address_part

[Sandra Loosemore]

Ian Lance Taylor wrote:

> Sandra Loosemore <sandra@codesourcery.com> writes:
> 
>> Richard's understanding of this issue matches mine.  FWIW, here's the
>> version of the patch I'm currently testing.  Assuming this passes, is
>> it OK to commit?
> 
> Yes.

OK, committed after bootstrap and testing on i686-pc-linux-gnu.

-Sandra

Re: PATCH: bug in find_reloads_address_part

[Eric Botcazou]

> I have to agree that this appears to fix an obvious bug.  However,
> it's rather unnerving that this bug appears to have existed since the
> beginning of time.

I just stumbled on it with a GCC 4.1-based Ada compiler on Alpha/Tru64!  But 
it's a very pathological case.  Sandra's fix works fine there too AFAICS.

-- 
Eric Botcazou