Re: [PATCH 0/2] Proof-of-concept towards removal of the "cfun" global

[Richard Biener <richard dot guenther at gmail dot com>]

On Sat, May 25, 2013 at 9:33 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Here's an idea that could make it easier to remove the "cfun" global.
>>
>> "cfun" is a major piece of global state within gcc: it's the 5th most
>> accessed variable in the build (accessed in ~4600 places within one stage
>> of a build, based on [1]).   This is an obstacle to making gcc's code be
>> usable as a library.
>
> Yep, note that cfun is not the only beast of this.  For 99% of backend it needs
> to be kept in sync with current_function_decl pointing to the declaration such
> that DECL_STRUCT_FUNCTION (current_function_decl) = cfun.
> There is also implicit notion of "current cgraph node" that is accessed by
> cgraph_get_node (current_function_decl) many times that can get expensive
> since it leads to hashtable lookup.
> Finally there is "crtl" that is macro hidding a global variable x_rtl
> data for RTL data of the currently compiled function.
>
> In many ways these four are all closely related and data in between them
> are not distributed in very engineered way.

Note that ideally there exists a single global variable per compilation phase.
Frontends would use current_function_decl (as struct function is not available
very early) while the middle-end and target would use cfun.

Through cfun you can reach its decl and you should ideally be able to reach
the cgraph node, too.

>> I can think of three approaches to "cfun":
>> (a) status quo: a global variable, with macros to prevent direct
>>     assignment, and an API for changing cfun.
>
> We have push_cfun/pop_cfun for that.  If you did not look into what they
> really do, you may be disappointed. They are huge&expensive beasts switching
> a lot of global state in compiler for optimize attribute.  So it means that
> on the track removing them we will need to cleanup this area, too.

Yeah ...

>> (b) have a global "context" or "universe" object, and put cfun in
>>     there (perhaps with tricks to be able to make this a singleton in a
>>     non-library build, optimizing away the context lookups somehow
>>     - see [2] for discussion on this)
>> (c) go through all of the places where cfun is used, and somehow ensure
>>     that they're passed in the data they need.  Often it's not the
>>     function that's used, but its cfg.
>>
>> I think (c) is the ideal from a modularity perspective (it unlocks the
>> ability to have optimization passes be working on different functions in
>> different threads), but the most difficult.
>
> (c) is direction I was trying to push IPA code to in longer run.  We need
> a single object holding "function" and that should be passed to most
> of the macros.
>
> I am not entirely happy with contents of the "struct function" at all
> (it is kind of kitchen sink for all the data we randomly need to associate
> with function), but it is closest to it. Most of IPA node considers the
> ultimate function pointer to be the cgraph node however.
>
> We do not want to melt those all thogether, since the data in there have different
> lifetime.  In particular struct_function is not in memory during WPA.

I agree to all of the above.

It would be interesting to have a static analyzer compute what are the most
executed functions that ultimately end up referencing cfun ...

>>
>> One part of the puzzle is that various header files in the build define
>> macros that reference the "cfun" global, e.g.:
>>
>>   #define n_basic_blocks         (cfun->cfg->x_n_basic_blocks)
>>
>> This one isn't in block caps, which might mislead a new contributor into
>> thinking it's a variable, rather than a macro, so there may be virtue in
>
> Yep yo umany notice there are already _FN variants for that.  The macors
> are lower case since historically they were variables. I agree they should
> be replaced.

Yes, explicit "cfun" usage is way better than implicit one - especially because
we still have implicit uses in functions that are already being passed the
function context as parameter ...

>> removing these macros for that reason alone.  (I know that these confused
>> me for a while when I first started writing my plugin) [3]
>>
>> So I had a go at removing these macros, to make usage of "cfun" be
>> explicit.
>>
>> I wrote a script to do most of the gruntwork automatically: [4]
>>
>> The following patches introduce accessor methods to control_flow_graph,
>> then remove all of the macros that reference cfun from basic-block.h,
>> replacing all of the places that use them with explicit uses of
>> "cfun->cfg->get_foo ()" as appropriate.  There are various other headers
>> that define macros that use cfun, but I thought I'd post this to get a
>> sense of how maintainers feel about this approach.
>>
>> I notice that we're constantly accessing:
>>
>>   some loop
>>     {
>>       ...
>>       use cfun->cfg->x_some_field;
>>       ...
>>     }
>>
>> Would it potentially be faster to replace some of these with:
>>
>>   control_flow_graph &cfg = *cfun->cfg;
>>   some loop
>>     {
>>       ...
>>       use cfg.get_some_field () // assuming inlining of accessor
>>       ...
>>     }
>>
>> to avoid constantly derefing cfun->cfg?

I think that was one reason crtl was introduced ... to avoid an indirection
through cfun.

>>  (That said, would
>>  -fstrict-aliasing
>> be able to note that cfun->cfg doesn't change, or in a non-LTO build I'm
>> guessing it can't make that assumption if the loop calls into functions it
>> can't see inside?).
>
> I like the general direction (i.e. it is what I was hoping for to happen for
> years, but was too lazy to enforce it more curefully).
> We should have single "function" object that is passed around and it should
> have substructures or pointers to all the various stuff we hold about function
> globally (gimple body, cfg, loop tree, profile, IPA data).
>
> Just some random toughts. I epxect there will be more discussion ;)

I indeed welcome the effort to make all 'cfun' uses explicit.  Even more welcome
is reducing the number of 'cfun' references ;)  Like giving all gimple/rtl pass
execute () / gate () functions a struct function argument (or even
better abstract
that so we can add more context transparently later, like dump_file /
dump_flags).

Richard.

> Honza