Fix PR 60040

Bernd Schmidt writes:

> On 04/15/2016 02:52 PM, Senthil Kumar Selvaraj wrote:
>>
>> For both testcases in the PR, reload fails to take into account that
>> FP-SP elimination can no longer be performed, and tries to find reload
>> regs for an rtx generated when FP-SP elimination was valid.
>>
>> 1. reload initializes elim table with FP->SP elimination enabled.
>> 2. alter_reg for a pseudo allocates a stack slot for the pseudo, and sets
>>     reg_equiv_memory_loc to frame_pointer_rtx plus offset. It also sets
>>     something_was_spilled to true.
>> 3. The main reload loop starts, and it resets something_was_spilled to false.
>> 4. reload calls eliminate_regs for the pseudo and sets reg_equiv_address to
>>     (mem(SP + offset)).
>> 5. calculate_needs_all_insns pushes a reload for SP (for the AVR target,
>>     SP cannot be a pointer reg - it needs to be reloaded into X Y or Z regs).
>> 6. update_eliminables_and_spill calls targetm.frame_pointer_required,
>>     which returns true. That causes can_eliminate for FP->SP to be reset
>>     to zero, and FP to be added to bad_spill_regs_global. For the AVR,
>>     FP is Y, one of the 3 pointer regs. reload also notes that something
>>     has changed, and that the loop needs to run again.
>> 7. reload still calls select_reload_regs, and find_regs fails to find a
>>     pointer reg to reload SP, which is unnecessary as FP->SP elimination
>>     had been disabled anyway in (6).
>>
>> IOW, reload fails to find pointer regs for an RTL expression that was
>> created when FP->SP elimination was true, even after it turns out that
>> the elimination can't be done after all. The patch tries to detect that
>> - if it knows the loop is going to run again, it silences the failure.
>>
>> Also note that at a different point in the loop, the reload loop starts
>> over if something_was_spilled (line 982-986). If set outside the reload
>> loop by alter_reg, it gets reset at (3) - not sure why. I'd think a
>> "continue" after update_eliminables_and_spill (line 1019-1022) would
>> also work - haven't tested it though.
>
> That's what I was going to ask next. I think if that works for you, I 
> think that's an approach that would make more sense.
>
> However, it looks like after this call, and the other one in the same 
> loop, should probably be calling finish_spills. It looks like an 
> oversight in the existing code that this doesn't happen for the existing 
> continue. Please try adding it in both places.

Tried that out - that works too. Bootstrap and regression tests on
x86_64 went fine, and there were no regressions for the avr target as
well - the extra three passes that showed up with the initial patch show
up now too.

I couldn't put a continue before select_reload_regs though.
save_call_clobbered_regs sets up a few things that are cleaned up in
delete_caller_save_insns a little later in the loop. So I only exclude
select_reload_regs - finish_spills and cleanup will happen in both cases.

Does this patch look ok? If yes, could you commit it for me please? I
don't have commit access.

Regards
Senthil

gcc/ChangeLog

2016-04-28  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/60040
	* reload1.c (reload): Call finish_spills before
	restarting reload loop. Skip select_reload_regs
	if update_eliminables_and_spill returns true.

gcc/testsuite/ChangeLog

2016-04-28  Sebastian Huber  <sebastian.huber@embedded-brains.de>
	Matthijs Kooijman  <matthijs@stdin.nl>
	Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/60040
	* gcc.target/avr/pr60040-1.c: New.
	* gcc.target/avr/pr60040-2.c: New.

Fix PR 60040

Commit Message

Comments

Patch