Fixed Regressions with "[committed] Use target-insns.def for prologue & epilogue insns"

> From: Richard Sandiford <rdsandiford@googlemail.com>
> Date: Wed, 1 Jul 2015 23:26:59 +0200

> Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
> >> From: Richard Sandiford <richard.sandiford@arm.com>
> >> Date: Tue, 30 Jun 2015 22:55:24 +0200
> >
> >> Bootstrapped & regression-tested on x86_64-linux-gnu and aarch64-linux-gnu.
> >> Also tested via config-list.mk.  Committed as preapproved.
> >> 
> >> Thanks,
> >> Richard
> >> 
> >> 
> >> gcc/
> >>         * defaults.h (HAVE_epilogue, gen_epilogue): Delete.
> >>         * target-insns.def (epilogue, prologue, sibcall_prologue): New
> >>         targetm instruction patterns.
> >>         * alias.c (init_alias_analysis): Use them instead of HAVE_*/gen_*
> >>         interface.
> >>         * calls.c (expand_call): Likewise.
> >>         * cfgrtl.c (cfg_layout_finalize): Likewise.
> >>         * df-scan.c (df_get_entry_block_def_set): Likewise.
> >>         (df_get_exit_block_use_set): Likewise.
> >>         * dwarf2cfi.c (pass_dwarf2_frame::gate): Likewise.
> >>         * final.c (final_start_function): Likewise.
> >>         * function.c (thread_prologue_and_epilogue_insns): Likewise.
> >>         (reposition_prologue_and_epilogue_notes): Likewise.
> >>         * reorg.c (find_end_label): Likewise.
> >>         * toplev.c (process_options): Likewise.
> >
> > I think this one -being the most fitting patch in the range
> > (225190:225210]- caused this regression for cris-elf:
> >
> > Running
> > /tmp/hpautotest-gcc1/gcc/gcc/testsuite/gcc.target/cris/torture/cris-torture.exp
> > ...
> > FAIL: gcc.target/cris/torture/no-pro-epi-1.c   -O3 -g  (internal compiler error)
> > FAIL: gcc.target/cris/torture/no-pro-epi-1.c   -O3 -g  (test for excess errors)
> >
> > This test checks that the -mno-prologue-epilogue option works,
> > whose semantics is supposedly self-explanatory.
> 
> Well, yes and no :-)

Hm...I take that as an affirmation on the regression but perhaps
a "no" to some of the my statements...

>  The crash is coming from the code that outputs
> dwarf CFI information.  The code that records this information is skipped
> for targets without rtl prologues, with the comment:
> 
>   /* Targets which still implement the prologue in assembler text
>      cannot use the generic dwarf2 unwinding.  */
> 
> That seems accurate.  So what's -mno-prologue-epilogue supposed to do
> wrt CFI entries?  Should it output empty entries or none at all?

A big "whatever" on that one.  Debugging and omitting prologue
and epilogue is not something I find reason to spend time on
other than making sure there are no crashes.

> The first-order reason for the failure is that the code used to be
> conditional on #ifndef HAVE_prologue and didn't care what HAVE_prologue
> itself evaluated to.  So the condition on the pattern wasn't actually
> tested.

Not completely true: there was inconsistency between uses of
#ifdef and if (HAVE_prologue).

> Which I suppose leads to the question: does !HAVE_prologue when "prologue"
> is defined mean "I know how to output rtl prologues, but the prologue
> for this function is empty" or "I'll output the prologue as text rather
> than rtl".  I think it logically means the second.

Agreed.

> The condition says
> whether the pattern can be used; if the pattern can be used but happens
> to generate no code then it just outputs no instructions (which is pretty
> common for prologues in leaf functions).
> 
> The port seems to hedge its bets here.  It has both:
> 
> (define_expand "prologue"
>   [(const_int 0)]
>   "TARGET_PROLOGUE_EPILOGUE"
>   "cris_expand_prologue (); DONE;")
> 
> and:
> 
> void
> cris_expand_prologue (void)
> {
>   [...]
>   /* Don't do anything if no prologues or epilogues are wanted.  */
>   if (!TARGET_PROLOGUE_EPILOGUE)
>     return;

Yeah, a visit to the archive supports me thinking this was an
oversight, perhaps caused by the effects of the now fixed
inconsistency.

> which I guess means that the HAVE_prologue condition wasn't being
> consistently tested.  Now that it is: is -mno-prologue-epilogue
> just supposed to generate empty prologues and epilogues, as implied
> by the cris.c code?  If so then removing the conditions on "prologue"
> and "epilogue" should work.  If not, then which of the targetm.have_prologue ()
> etc. conditions do you need to be true for -mno-prologue-epilogue?
> 
> (You have the distinction of having the only port with conditional
> prologue and epilogue patterns. :-))

Not any longer.  Also removed a stale comment.
This committed patch fixes the noted regressions, without
causing further regressions, testing cris-elf in a simulator.

gcc:
	* config/cris/cris.md ("epilogue"): Remove condition.
	("prologue"): Ditto.

brgds, H-P

Fixed Regressions with "[committed] Use target-insns.def for prologue & epilogue insns"

Commit Message

Comments

Patch