Re: [PATCH 4/6] Shrink-wrapping

[Bernd Schmidt <bernds at codesourcery dot com>]

Whee! Thanks for reviewing (reviving?) this old thing.

I should be posting an up-to-date version of this, but for the moment it
has to wait until dwarf2out is sorted out, and I'm rather busy with
other stuff. I hope to squeeze this in in the not too distant future.

I'll try to answer some of the questions now...

On 07/07/11 16:34, Richard Sandiford wrote:
> Bernd Schmidt <bernds@codesourcery.com> writes:
>> This adds the actual optimization, and reworks the JUMP_LABEL handling
>> for return blocks. See the introduction mail or the new comment ahead of
>> thread_prologue_and_epilogue_insns for more notes.
> 
> It seems a shame to have both (return) and (simple_return).

Yes, but the distinction exists and must be represented somehow - you
can have both in the same function.

> You said
> that we need the distinction in order to cope with targets like ARM,
> whose (return) instruction actually performs some of the epilogue too.
> It feels like the load of the saved registers should really be expressed
> in rtl, in parallel with the return.  I realise that'd prevent
> conditional returns though.  Maybe there's no elegant way out...

It certainly would make it harder to transform branches to conditional
returns. It would also require examining every port to see if it needs
changes to its return patterns. It probably only affects ARM though, but
that target is important enough that we should support the feature (i.e.
conditional returns that pop registers).

If we described conditional returns only as COND_EXEC maybe... AFAICT
only ia64, arm, frv and c6x have conditional return. I'll have to think
about it.

Note that some interface changes will be necessary in any case - passing
NULL as a new jump label simply isn't informative enough when
redirecting a jump; we must be able to distinguish between the two forms
of return at this level. So the ret_rtx/simple_return_rtx may turn out
to be the simplest solution after all.

> With the hidden loads, it seems like we'll have a situation in which the
> values of call-saved registers will appear to be different for different
> "real" incoming edges to the exit block.

Probably true, but I doubt we have any code that would notice. Can you
imagine anything that would care?

> Is JUMP_LABEL ever null after this change?  (In fully-complete rtl
> sequences, I mean.)  It looked like some of the null checks in the
> patch might not be necessary any more.

It shouldn't be, and it's possible that a few of these tests survived
when they shouldn't have.

> JUMP_LABEL also seems somewhat misnamed after this change; maybe
> JUMP_TARGET would be better?

Maybe. I dread the renaming patch though.

> It'd also be nice to get rid of all these big blocks of code that are
> conditional on preprocessor macros, but I realise you're just following
> existing practice in the surrounding code, so again it can be left to
> a future cleanup.

Yeah, this function is quite horrid - so many different paths through it.

However, it looks like the only target without HAVE_prologue is actually
pdp11, so we're carrying some unnecessary baggage for purely
retrocomputing purposes. Paul, can you fix that?

>>    ret_rtx = gen_rtx_fmt_ (RETURN, VOIDmode);
>> +  simple_return_rtx = gen_rtx_fmt_ (SIMPLE_RETURN, VOIDmode);
> 
> It'd be nice to s/ret_rtx/return_rtx/ for consistency, but that can
> happen anytime.

Unfortunately there's another macro called return_rtx.

>> +	&& df_regs_ever_live_p (regno))
>> +      return true;
> 
> This can be done as a follow-up, but it looks like df should be using
> a HARD_REG_SET here, and that we should be able to get at it directly.

For the df_regs_ever_live thing? Could change that, yes.

[...]
> AIUI, this prevents the optimisation for things like
> 
>   if (a) {
>     switch (b) {
>       case 1:
>         ...stuff that requires a frame...
>         break;
>       case 2:
>         ...stuff that requires a frame...
>         break;
>       default:
>         ...stuff that doesn't require a frame...
>         break;
>     }
>   }
> 
> The switch won't be in ANTIC, but it will have two successors that are.
> Is that right?
> 
> Would it work to do something like:
> 
[...]

IIRC the problem here is making sure to match up prologues and epilogues
- the latter should not occur on any path that had a prologue set up and
vice versa. I think something more clever would break on e.g.

   if (c)
     goto label;
   if (a) {
     switch (b) {
       case 1:
         ...stuff that requires a frame...
         break;
       case 2:
         ...stuff that requires a frame...
         break;
       default:
         ...stuff that doesn't require a frame...
	label:
         ...more stuff that doesn't require a frame...
         break;
     }
   }

If you add a prologue before the switch, two paths join at label where
one needs a prologue and the other doesn't.

> Does the "JUMP_LABEL (returnjump) = ret_rtx;" handle targets that
> use things like (set (pc) (reg RA)) as their return?  Probably worth
> adding a comment if so.

It simply must be a JUMP_INSN, right? I think we can assume that all
returns are JUMP_INSNS and fix any ports that break.


Bernd