Re: PATCH: bug in find_reloads_address_part

[Richard Sandiford <richard at codesourcery dot com>]

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