[PATCH 4/6] Shrink-wrapping

[Richard Sandiford]

Bernd Schmidt <bernds@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