[PATCH 4/6] Shrink-wrapping

[Bernd Schmidt]

On 08/03/11 17:38, Richard Sandiford wrote:
> 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?

It's important to distinguish between these names as rtxes that can
occur in instruction patterns, and a use as a standard pattern name.
When a "return" pattern is generated, it should either fail or expand to
something that performs both the epilogue and the return. A
"simple_return" expands to something that performs only the return.

Most targets allow "return" patterns only if the epilogue is empty. In
that case, "return" and "simple_return" can expand to the same insn; it
does not matter if that insn uses "simple_return" or "return", as they
are equivalent in the absence of an epilogue. It would be slightly nicer
to use "simple_return" in the patterns everywhere except ARM, but ports
don't need to be changed.

> Do they need both md patterns (but potentially using the same rtx
> underneath)?

The "return" standard pattern is needed for the existing optimizations
(inserting returns in-line rather than jumping to the end of the
function). Typically, it always fails if the function needs an epilogue,
except in the ARM case.
For shrink-wrapping to work, a port needs a "simple_return" pattern,
which the compiler can use even if parts of the function need an
epilogue. So yes, they have different purposes.

> 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.

This is not accurate for the rtx code. It is mostly accurate for the
standard pattern name. A simple_return rtx may occur just after an
epilogue, i.e. on a path that has passed through the prologue.

Even for the simple_return pattern, I'm not sure reorg.c couldn't
introduce new expansions in a location after both prologue and epilogue.

>   Like @code{(return)}, @code{(simple_return)} may also occur in
>   conditional jumps.
> 
> You need to document the simple_return pattern in md.texi too.

I was trying to update the documentation to only the current state after
the patch. The thinking was that without shrink-wrapping, nothing
generates this pattern, so documenting it would be misleading.
However, with the mips changes in this version of the patch, reorg.c
does make use of this pattern, so I've added documentation

>> @@ -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?

That probably was the idea. Looking at it again, there's one case at the
bottom of the loop which may be safe, but given that there were no code
generation differences with the patch on three targets with
define_delay, I've done:

> If so, I think you should remove the immediately-following.
> "if (!ANY_RETURN_P (target_label))" condition and reindent the body.

this.

> 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).

Done.

> 
>> +#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.

Restructured.

> (define_code_iterator any_return [return simple_return])
> 
> and just change the appropriate returns to any_returns.

I've done this a bit differently - to show that it can be done, I've
changed mips to always emit simple_return rtxs, even for "return"
patterns (no code generation changes observed again).

This version regression tested on mips64-elf, c/c++/objc.


Bernd
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: sw-part2.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20110824/288ba1a7/attachment.ksh>