Re: Out-of-order update of new_spill_reg_store[]

[Richard Sandiford <rdsandiford at googlemail dot com>]

Bernd Schmidt <bernds@codesourcery.com> writes:
> On 10/11/11 14:35, Richard Sandiford wrote:
>> No, reload 1 is inherited by a later instruction.  And it's inherited
>> correctly, in terms of the register contents being what we expect.
>> (Reload 1 is the one that survives to the end of the instruction's
>> reload sequence.  Reload 2, in contrast, is clobbered by reload 1,
>> so could not be inherited.  So when we record inheritance information
>> in emit_reload_insns, reload_reg_reaches_end_p correctly stops us
>> from recording reload 2 but allows us to record reload 1.)
>> 
>> The problem is that we record the wrong instruction for reload 1.
>> We say that reload 1 is performed by the instruction that performs
>> reload 2.  So spill_reg_store[] contains the instruction for reload 2
>> rather than the instruction for reload 1.  We delete it in
>> delete_output_reload at the point of inheritance.
>
> Ok. So, would the minimal fix of testing !new_spill_reg_store[..] before
> writing to it also work? Seems to me this would cope with the
> out-of-order writes by only allowing the first.
>
> If so, then I think I'd prefer that, but we could gcc_assert
> (reload_reg_reaches_end (..)) as a bit of a verification of that function.

I don't think the assert would be safe.  We could have similar reuse in
cases where the first reload (in rld order) is a double-register value
starting in $4 and the second reload uses just $5.  In that case, the first
reload will have set new_spill_reg_store[4], so new_spill_reg_store[5] will
still be null.  But $5 in the second reload won't survive until the end of
the sequence.  So we'd try to set new_spill_reg_store[5] and trip the assert.

IMO it's a choice between just checking for null and not asserting
(if that really is safe, since we'll be storing instructions that
don't actually reach the end of the reload sequence), or checking
reload_reg_reaches_end.  I prefer the second, since it seems more
direct, and matches the corresponding code in emit_reload_insns.

Richard