Re: [PATCH 0/9] separate shrink-wrapping

[Segher Boessenkool <segher@kernel.crashing.org>]

On Thu, Jun 09, 2016 at 10:12:53AM -0600, Jeff Law wrote:
> I'm going to largely let Bernd own the review on this.  Just a few comments.
> 
> I certainly like the concept.  My mental model is that parts of the 
> prologue might sink further than other parts of the prologue.  It's not 
> an exact match, but I think it's a "close enough" mental model.

The "normal", "main" shrink-wrapping sinks the prologue, duplicating
blocks that can be run both with and without prologue.  It always
keeps one prologue, and any path through the function will execute the
prologue at most once.

Separate shrink-wrapping can put the prologue pieces in more than one
spot (each), and also execute them more than once.  But it does not
copy blocks, that can explode the code size too much if there are many
concerns.  The standard example:

      |
      1
     / \
    2   3
     \ /
      4
     / \
    5   6
     \ /
      7
      |

where 3 and 6 have some concern.  If 3 and 6 together have a lower
execution frequency than 1 does, it is better to put prologues and
epilogues around 3 and 6; otherwise, it is better to have a prologue
before 1 and an epilogue after 7.

> It looks like the "concerns" are all target defined and its the target's 
> responsibility to deal with dependencies between the "concerns".  Right? 

Exactly.

There are not many dependencies in general (except for the obvious
"setting up a stack frame has to come before almost everything else").

I have found no reasonably simple way yet to have the target inform the
generic code about the dependencies, and nothing uses it so far anyway.
So I just punt the responsibility it to the target code, for now anyway.

>  (BTW, I think "concerns" is a particularly poor name, perhaps 
> "reasons" would be better?).

I think that's even worse.  "pieces", maybe?

> Right now it looks like shrink_wrapped_separate essentially says "we did 
> something special here" -- while I don't think it's necessary for this 
> patch, describing what we did might be better.  Essentially different 
> paths would have a set of prologue/epilogue attributes that apply to 
> that path.

It is used by a few later passes to say "don't touch some stuff, you do
not have all the info you need".  This is analogous to the existing
"shrink_wrapped" flag.

> This would be useful for things like cfgcleanup where you could join 
> noreturn calls more aggressively.   Though this may not matter in any 
> significant way in practice.

Oh, it already tried to join such paths more aggressively.  Which then
blows up big time in the usual dwarf2cfi spot.

> There may be enough commonality that we can promote some "concerns" from 
> target dependent to target independent -- I've often wondered if we 
> could handle a fair amount of prologue/epilogue generation in target 
> independent ways.    For simple targets, I wouldn't be terribly 
> surprised if all prologue/epilogue generation could be handled 
> generically and they'd get separate shrink-wrapping "for free".  But 
> that's all blue-sky.

I don't think that will work to well, unfortunately.


Segher