Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function

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

On Wed, Aug 15, 2012 at 5:21 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Fri, Aug 10, 2012 at 04:57:41PM +0200, Eric Botcazou wrote:
>> > - ada/gcc-interface/utils.c:rest_of_subprog_body_compilation calls
>> >   dump_function which in turns calls dump_function_to_file which calls
>> >   push_cfun.  But Ada front end has its idea of the
>> >   current_function_decl and there is no cfun which is an inconsistency
>> >   which makes push_cfun assert fail.  I "solved" it by temporarily
>> >   setting current_function_decl to NULL_TREE.  It's just dumping and I
>> >   thought that dump_function should be considered middle-end and thus
>> >   middle-end invariants should apply.
>>
>> If you think that calling dump_function from rest_of_subprog_body_compilation
>> is a layering violation, I don't have a problem with replacing it with a more
>> "manual" scheme like the one in c-family/c-gimplify.c:c_genericize, provided
>> that this yields roughly the same output.
>
> Richi suggested on IRC that I remove the push/pop_cfun calls from
> dump_function_to_file.  The only problem seems to be
> dump_histograms_for_stmt (buried in a dump_bb call) which needs a
> function to get the value histograms.  Richi suggested to replace the
> per function hash with one global one.  I tried that and bumped into
> two problems: First, I did not really know when to deallocate the
> global hash and all the histograms.  More importantly, the current
> gimple verifier (verify_gimple_in_cfg) checks that there are no "dead
> histograms" in the hash corresponding to statements no longer in the
> function.  With one global hash, it would be very cumbersome to do
> this check (add cfun to each histogram?  only when checking is
> enabled?).  Hash tables are now in flux anyway so I postponed that
> work until the interface settles down.  Nevertheless, I guess I need a
> confirmation from maintainers that I can remove the checking if I
> continue this way.
>
> When I looked at what would need to be changed if I added a function
> parameter to dump_bb, the work list grew very large very quickly.  It
> is probably doable, but the change will be quite big, so if anyone
> thinks it is a bad idea, please email me to stop.
>
> If I manage to change the dumping in one of the two ways, the call
> will not be a layering problem any more as it probably should not be.
> At the moment, it probably is.  We'll see.

If dumping a statement needs the containing function then we need to
either pass that down, provide a way to get from statement to function,
or stop requiring the function.  Making the hash global is choice three
(deallocating the hash would be when compilation is finished, in which
case you can double-check that it is empty and preserve above checking).
Option one is ok as well, option two is probably either unreliable (going
from gimple_block to function from function scope - but maybe who cares
for dumping?) or expensive (add a pointer from basic_block to struct
function).  I'm fine with both option three (would even save a pointer in
struct function!) or one.

Other opinions?

Thanks,
Richard.

> Thanks,
>
> Martin