Eric Botcazou <ebotca...@adacore.com> writes:
> Hi,
>
> this is a regression present on all active branches since the controversial
> get_initial_register_offset stuff was added to rtlanal.c some time ago, and
> visible in the testsuite on PowerPC/Linux under the form of gnat.dg/opt73.adb
> timing out at run time.
>
> The problem is that the compiler generates code that doesn't save the frame
> pointer before clobbering it, because rs6000_stack_info computes a wrong
> final
> (post-reload) stack layout. The scenario is as follows: LRA decides to use
> the frame pointer, sets reload_completed to 1 at the end and then does:
>
> /* We've possibly turned single trapping insn into multiple ones. */
> if (cfun->can_throw_non_call_exceptions)
> {
> auto_sbitmap blocks (last_basic_block_for_fn (cfun));
> bitmap_ones (blocks);
> find_many_sub_basic_blocks (blocks);
> }
>
> But find_many_sub_basic_blocks calls control_flow_insn_p, which in turn can
> call rtx_addr_can_trap_p_1, which can call get_initial_register_offset, which
> uses INITIAL_ELIMINATION_OFFSET, IOW rs6000_initial_elimination_offset, which
> calls rs6000_stack_info. But at this point the DF information hasn't been
> updated so the frame pointer isn't detected as live by df_regs_ever_live_p.
Doesn't the frame have to be laid out correctly during the final reload
subpass anyway, in order to get the right elimination choices and
elimination offsets? If so, I'm not sure what the " && info->reload_completed"
check in rs6000_stack_info achieves. It seems like that's at least partly
responsible for the problem.
> You may think that the fix is just to set reload_completed to 1 after the
> above code in lra, but that's not sufficient because the same issue can arise
> from the do_reload function:
>
> if (optimize)
> cleanup_cfg (CLEANUP_EXPENSIVE);
>
> when checking is enabled, because cleanup_cfg can calls control_flow_insn_p
> and then eventually rtx_addr_can_trap_p_1. In other words, we would need
> to set reload_completed to 1 only after the above code, which is very late.
> As a matter of fact, that's not possible for old reload itself because of:
>
> /* We must set reload_completed now since the cleanup_subreg_operands call
> below will re-recognize each insn and reload may have generated insns
> which are only valid during and after reload. */
> reload_completed = 1;
>
> So, barring the removal of the get_initial_register_offset stuff, the only
> simple fix is probably to prevent it from calling into the back-end too
> early,
> for example with the attached fixlet. Tested on x86-64 and PowerPC/Linux.
>
> Thoughts? Where do we want to fix this?
>
>
> * rtlanal.c (get_initial_register_offset): Fall back to the raw estimate
> as long as the epilogue isn't completed.
I realise you've already applied it and that you saw it as a hack too,
but this seems like a bit too much. IMO a cleaner fix would be to define
TARGET_COMPUTE_FRAME_LAYOUT for rs6000.
I guess that approach means that TARGET_COMPUTE_FRAME_LAYOUT isn't really
optional though.
Thanks,
Richard
