Bernd Schmidt <ber...@codesourcery.com> writes: > +@findex simple_return > +@item (simple_return) > +Like @code{(return)}, but truly represents only a function return, while > +@code{(return)} may represent an insn that also performs other functions > +of the function epilogue. Like @code{(return)}, this may also occur in > +conditional jumps.
Sorry, I've forgotton the outcome of the discussion about what happens on targets whose return expands to the same code as their simple_return. Do the targets still need both "return" and "simple_return" rtxes? Do they need both md patterns (but potentially using the same rtx underneath)? I ask because the rtl.def comment implies that those targets still need both expanders and both rtxes. If that's so, I think it needs to be mentioned here too. E.g. something like: Like @code{(return)}, but truly represents only a function return, while @code{(return)} may represent an insn that also performs other functions of the function epilogue. @code{(return)} only occurs on paths that pass through the function prologue, while @code{(simple_return)} only occurs on paths that do not pass through the prologue. Like @code{(return)}, @code{(simple_return)} may also occur in conditional jumps. You need to document the simple_return pattern in md.texi too. > @@ -231,6 +234,15 @@ first_active_target_insn (rtx insn) > return next_active_insn (insn); > } > > +/* Return true iff INSN is a simplejump, or any kind of return insn. */ > + > +static bool > +simplejump_or_return_p (rtx insn) > +{ > + return (JUMP_P (insn) > + && (simplejump_p (insn) || ANY_RETURN_P (PATTERN (insn)))); > +} Maybe better in jump.c? I'll leave it up to you though. > @@ -346,23 +358,29 @@ insn_sets_resource_p (rtx insn, struct r > > ??? There may be a problem with the current implementation. Suppose > we start with a bare RETURN insn and call find_end_label. It may set > - end_of_function_label just before the RETURN. Suppose the machinery > + function_return_label just before the RETURN. Suppose the machinery > is able to fill the delay slot of the RETURN insn afterwards. Then > - end_of_function_label is no longer valid according to the property > + function_return_label is no longer valid according to the property > described above and find_end_label will still return it unmodified. > Note that this is probably mitigated by the following observation: > - once end_of_function_label is made, it is very likely the target of > + once function_return_label is made, it is very likely the target of > a jump, so filling the delay slot of the RETURN will be much more > difficult. */ > > static rtx > -find_end_label (void) > +find_end_label (rtx kind) Need to document the new parameter. > { > rtx insn; > + rtx *plabel; > + > + if (kind == ret_rtx) > + plabel = &function_return_label; > + else > + plabel = &function_simple_return_label; I think it'd be worth a gcc_checking_assert that ret == simple_return_rtx in the other case. > - /* Put the label before an USE insns that may proceed the > + /* Put the label before an USE insns that may precede the > RETURN insn. */ Might as well fix s/an USE/any USE/ too while you're there > @@ -3498,6 +3506,8 @@ relax_delay_slots (rtx first) > continue; > > target_label = JUMP_LABEL (delay_insn); > + if (target_label && ANY_RETURN_P (target_label)) > + continue; > > if (!ANY_RETURN_P (target_label)) > { This doesn't look like a pure "handle return as well as simple return" change. Is the idea that every following test only makes sense for labels, and that things like: && prev_active_insn (target_label) == insn (to pick just one example) are actively dangerous for returns? If so, I think you should remove the immediately-following. "if (!ANY_RETURN_P (target_label))" condition and reindent the body. > @@ -3737,13 +3753,27 @@ make_return_insns (rtx first) > for (insn = first; insn; insn = NEXT_INSN (insn)) > { > int flags; > + rtx kind, real_label; > > /* Only look at filled JUMP_INSNs that go to the end of function > label. */ > if (!NONJUMP_INSN_P (insn) > || GET_CODE (PATTERN (insn)) != SEQUENCE > - || !JUMP_P (XVECEXP (PATTERN (insn), 0, 0)) > - || JUMP_LABEL (XVECEXP (PATTERN (insn), 0, 0)) != > end_of_function_label) > + || !JUMP_P (XVECEXP (PATTERN (insn), 0, 0))) > + continue; > + > + if (JUMP_LABEL (XVECEXP (PATTERN (insn), 0, 0)) == > function_return_label) > + { > + kind = ret_rtx; > + real_label = real_return_label; > + } > + else if (JUMP_LABEL (XVECEXP (PATTERN (insn), 0, 0)) > + == function_simple_return_label) > + { > + kind = simple_return_rtx; > + real_label = real_simple_return_label; > + } > + else > continue; > > pat = PATTERN (insn); Given what you said about JUMP_LABEL sometimes being null, I think we need either (a) to check whether each *_return_label is null before comparing it with JUMP_LABEL, or (b) to ensure that we're dealing with a jump to a label. (b) seems neater IMO (as a call to jump_to_label_p). > +#if defined HAVE_return || defined HAVE_simple_return > + if ( > #ifdef HAVE_return > - if (HAVE_return && end_of_function_label != 0) > + (HAVE_return && function_return_label != 0) > +#else > + 0 > +#endif > +#ifdef HAVE_simple_return > + || (HAVE_simple_return && function_simple_return_label != 0) > +#endif > + ) > make_return_insns (first); > #endif Eww. Given that make_return_insns clears the *return_labels, it's probably more readable just to have two conditional calls: #ifdef HAVE_return if (HAVE_return && function_return_label != 0) make_return_insns (first); #endif #ifdef HAVE_simple_return if (HAVE_simple_return && function_simple_return_label != 0) make_return_insns (first); #endif I'll leave it up to you though. > Index: gcc/emit-rtl.c > =================================================================== > --- gcc/emit-rtl.c (revision 176881) > +++ gcc/emit-rtl.c (working copy) > @@ -2518,6 +2518,7 @@ verify_rtx_sharing (rtx orig, rtx insn) > case PC: > case CC0: > case RETURN: > + case SIMPLE_RETURN: > case SCRATCH: > return; > /* SCRATCH must be shared because they represent distinct values. */ Given Alan's patch, I suppose you also need cases for copy_rtx_if_shared_1, copy_insn_1 and mark_used_flags. (Sorry about being wise after the fact here.) > Index: gcc/config/mips/mips.md > =================================================================== > --- gcc/config/mips/mips.md (revision 176879) > +++ gcc/config/mips/mips.md (working copy) > @@ -5724,6 +5724,18 @@ (define_insn "*return" > [(set_attr "type" "jump") > (set_attr "mode" "none")]) > > +(define_expand "simple_return" > + [(simple_return)] > + "!mips_can_use_return_insn ()" > + { mips_expand_before_return (); }) > + > +(define_insn "*simple_return" > + [(simple_return)] > + "!mips_can_use_return_insn ()" > + "%*j\t$31%/" > + [(set_attr "type" "jump") > + (set_attr "mode" "none")]) > + > ;; Normal return. > > (define_insn "return_internal" > @@ -5731,6 +5743,14 @@ (define_insn "return_internal" > (use (match_operand 0 "pmode_register_operand" ""))] > "" > "%*j\t%0%/" > + [(set_attr "type" "jump") > + (set_attr "mode" "none")]) > + > +(define_insn "simple_return_internal" > + [(simple_return) > + (use (match_operand 0 "pmode_register_operand" ""))] > + "" > + "%*j\t%0%/" > [(set_attr "type" "jump") > (set_attr "mode" "none")]) Please add: (define_code_iterator any_return [return simple_return]) and just change the appropriate returns to any_returns. The rtl and MIPS bits look good to me otherwise. Richard