Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
Hi,
On Wed, Aug 22, 2012 at 03:37:48PM +0200, Richard Guenther wrote:
On Wed, Aug 22, 2012 at 3:04 PM, Martin Jambor mjam...@suse.cz wrote:
On Tue, Aug 21, 2012 at 01:30:47PM +0200, Richard Guenther wrote:
On Tue, Aug 21, 2012 at 1:27 PM, Martin Jambor mjam...@suse.cz wrote:
On Wed, Aug 15, 2012 at 05:21:04PM +0200, Martin Jambor 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
Yesterday I actually tried and it is not the only problem. Another
one is dump_function_to_file-dump_bb-maybe_hot_bb_p which uses cfun
to read profile_status. There may be others, this one just blew up
first when I set cfun to NULL. And in future someone is quite likely
to need cfun to dump something new too.
At the same time, re-implementing dumping
c-family/c-gimplify.c:c_genericize when dump_function suffices seems
ugly to me.
So I am going to declare dump_function a front-end interface and use
set_cfun in my original patch in dump_function_to_file like we do in
other such functions.
I hope that will be OK. Thanks,
Setting cfun has side-effects of switching target stuff which might have
code-generation side-effects because of implementation issues we have
with target/optimize attributes. So I don't think cfun should be changed
just for dumping.
Can you instead just set current_function_decl and access
struct function via DECL_STRUCT_FUNCTION in the dumpers then?
After all, it it is a front-end interface, the frontend way of saying
this is the current function is to set current_function_decl, not the
middle-end cfun.
Like the following? Tested and bootstrapped on x86_64-linux, it does
help avoid the ada hunk in my previous patch. OK for trunk?
Ok if nobody complains - btw, I think you miss to restore
current_function_decl
here:
*** /tmp/HcgoTd_tree-cfg.c Wed Aug 22 15:02:30 2012
--- gcc/tree-cfg.c Wed Aug 22 11:53:02 2012
*** move_sese_region_to_fn (struct function
*** 6632,6650
*/
void
! dump_function_to_file (tree fn, FILE *file, int flags)
{
! tree arg, var;
struct function *dsf;
bool ignore_topmost_bind = false, any_var = false;
basic_block bb;
tree chain;
! bool tmclone = TREE_CODE (fn) == FUNCTION_DECL decl_is_tm_clone (fn);
! fprintf (file, %s %s(, current_function_name (),
! tmclone ? [tm-clone] : );
! arg = DECL_ARGUMENTS (fn);
while (arg)
{
print_generic_expr (file, TREE_TYPE (arg), dump_flags);
--- 6632,6652
*/
void
! dump_function_to_file (tree fndecl, FILE *file, int flags)
{
! tree arg, var, old_current_fndecl = current_function_decl;
struct function *dsf;
bool ignore_topmost_bind = false, any_var = false;
basic_block bb;
tree chain;
! bool tmclone = (TREE_CODE (fndecl) == FUNCTION_DECL
! decl_is_tm_clone (fndecl));
! struct function *fun = DECL_STRUCT_FUNCTION (fndecl);
! current_function_decl = fndecl;
! fprintf (file, %s %s(, function_name (fun), tmclone ? [tm-clone] :
);
! arg = DECL_ARGUMENTS (fndecl);
while (arg)
{
print_generic_expr (file, TREE_TYPE (arg), dump_flags);
*** dump_function_to_file (tree fn, FILE *fi
*** 6659,6689
fprintf (file, )\n);
if (flags TDF_VERBOSE)
! print_node (file, , fn, 2);
! dsf = DECL_STRUCT_FUNCTION (fn);
if (dsf (flags TDF_EH))
dump_eh_tree (file, dsf);
! if (flags TDF_RAW !gimple_has_body_p (fn))
{
! dump_node (fn, TDF_SLIM | flags, file);
return;
}
- /* Switch CFUN to point to FN. */
- push_cfun (DECL_STRUCT_FUNCTION (fn));
-
/* When GIMPLE is lowered, the variables are no longer available in
BIND_EXPRs, so display them separately. */
!
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
Hi, On Tue, Aug 21, 2012 at 01:30:47PM +0200, Richard Guenther wrote: On Tue, Aug 21, 2012 at 1:27 PM, Martin Jambor mjam...@suse.cz wrote: On Wed, Aug 15, 2012 at 05:21:04PM +0200, Martin Jambor 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 Yesterday I actually tried and it is not the only problem. Another one is dump_function_to_file-dump_bb-maybe_hot_bb_p which uses cfun to read profile_status. There may be others, this one just blew up first when I set cfun to NULL. And in future someone is quite likely to need cfun to dump something new too. At the same time, re-implementing dumping c-family/c-gimplify.c:c_genericize when dump_function suffices seems ugly to me. So I am going to declare dump_function a front-end interface and use set_cfun in my original patch in dump_function_to_file like we do in other such functions. I hope that will be OK. Thanks, Setting cfun has side-effects of switching target stuff which might have code-generation side-effects because of implementation issues we have with target/optimize attributes. So I don't think cfun should be changed just for dumping. Can you instead just set current_function_decl and access struct function via DECL_STRUCT_FUNCTION in the dumpers then? After all, it it is a front-end interface, the frontend way of saying this is the current function is to set current_function_decl, not the middle-end cfun. Like the following? Tested and bootstrapped on x86_64-linux, it does help avoid the ada hunk in my previous patch. OK for trunk? Thanks, Martin 2012-08-21 Martin Jambor mjam...@suse.cz * predict.c (maybe_hot_frequency_p): New parameter fun. Use its decl instead of current_function_decl, use profile_status_for_function and ENTRY_BLOCK_PTR_FOR_FUNCTION with fun instead of their cfun variants. (maybe_hot_count_p): New parameter fun, use profile_status_for_function instead of its cfun_variant. (maybe_hot_bb_p): New parameter fun, checking-assert it, pass it to all callees. (maybe_hot_edge_p): Pass cfun to maybe_hot_count_p and maybe_hot_frequency_p. (probably_never_executed_bb_p): New parameter fun, use its decl instead of current_function_decl. (optimize_bb_for_size_p): Pass cfun to maybe_hot_bb_p. (rtl_profile_for_bb): Likewise. (compute_function_frequency): Pass cfun to maybe_hot_bb_p and probably_never_executed_bb_p. * tree-ssa-operands.c (ssa_operands_active): New operator fun. Use it instead of cfun. (update_stmt_operands): Pass cfun as an argument of ssa_operands_active. (swap_tree_operands): Likewise. * gimple-iterator.c (update_modified_stmt): Likewise. (update_modified_stmts): Likewise. * tree-flow-inline.h (delink_stmt_imm_use): Likewise. * tree-ssa.c (delete_tree_ssa): Likewise. * bb-reorder.c (bb_to_key): Pass cfun to probably_never_executed_bb_p. (push_to_next_round_p): Likewise. (find_rarely_executed_basic_blocks_and_crossing_edges ): Likewise. * cfg.c: Inlude tree.h. (check_bb_profile): Use profile_status_for_function, EXIT_BLOCK_PTR_FOR_FUNCTION and ENTRY_BLOCK_PTR_FOR_FUNCTION with DECL_STRUCT_FUNCTION (current_function_decl) instead of their cfun variants. (dump_bb_info): Pass DECL_STRUCT_FUNCTION (current_function_decl) to maybe_hot_bb_p and probably_never_executed_bb_p. * gimple-pretty-print.c (gimple_dump_bb_buff): Checking-assert that DECL_STRUCT_FUNCTION (current_function_decl) is not NULL. Pass it to dump_histograms_for_stmt. (dump_gimple_mem_ops): Pass DECL_STRUCT_FUNCTION (current_function_decl) as an argument to dump_gimple_mem_ops. * tree-cfg.c (dump_function_to_file): Rename parameter fn to fndecl. Do not change cfun. Change and
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Wed, Aug 22, 2012 at 3:04 PM, Martin Jambor mjam...@suse.cz wrote:
Hi,
On Tue, Aug 21, 2012 at 01:30:47PM +0200, Richard Guenther wrote:
On Tue, Aug 21, 2012 at 1:27 PM, Martin Jambor mjam...@suse.cz wrote:
On Wed, Aug 15, 2012 at 05:21:04PM +0200, Martin Jambor 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
Yesterday I actually tried and it is not the only problem. Another
one is dump_function_to_file-dump_bb-maybe_hot_bb_p which uses cfun
to read profile_status. There may be others, this one just blew up
first when I set cfun to NULL. And in future someone is quite likely
to need cfun to dump something new too.
At the same time, re-implementing dumping
c-family/c-gimplify.c:c_genericize when dump_function suffices seems
ugly to me.
So I am going to declare dump_function a front-end interface and use
set_cfun in my original patch in dump_function_to_file like we do in
other such functions.
I hope that will be OK. Thanks,
Setting cfun has side-effects of switching target stuff which might have
code-generation side-effects because of implementation issues we have
with target/optimize attributes. So I don't think cfun should be changed
just for dumping.
Can you instead just set current_function_decl and access
struct function via DECL_STRUCT_FUNCTION in the dumpers then?
After all, it it is a front-end interface, the frontend way of saying
this is the current function is to set current_function_decl, not the
middle-end cfun.
Like the following? Tested and bootstrapped on x86_64-linux, it does
help avoid the ada hunk in my previous patch. OK for trunk?
Ok if nobody complains - btw, I think you miss to restore current_function_decl
here:
*** /tmp/HcgoTd_tree-cfg.c Wed Aug 22 15:02:30 2012
--- gcc/tree-cfg.c Wed Aug 22 11:53:02 2012
*** move_sese_region_to_fn (struct function
*** 6632,6650
*/
void
! dump_function_to_file (tree fn, FILE *file, int flags)
{
! tree arg, var;
struct function *dsf;
bool ignore_topmost_bind = false, any_var = false;
basic_block bb;
tree chain;
! bool tmclone = TREE_CODE (fn) == FUNCTION_DECL decl_is_tm_clone (fn);
! fprintf (file, %s %s(, current_function_name (),
! tmclone ? [tm-clone] : );
! arg = DECL_ARGUMENTS (fn);
while (arg)
{
print_generic_expr (file, TREE_TYPE (arg), dump_flags);
--- 6632,6652
*/
void
! dump_function_to_file (tree fndecl, FILE *file, int flags)
{
! tree arg, var, old_current_fndecl = current_function_decl;
struct function *dsf;
bool ignore_topmost_bind = false, any_var = false;
basic_block bb;
tree chain;
! bool tmclone = (TREE_CODE (fndecl) == FUNCTION_DECL
! decl_is_tm_clone (fndecl));
! struct function *fun = DECL_STRUCT_FUNCTION (fndecl);
! current_function_decl = fndecl;
! fprintf (file, %s %s(, function_name (fun), tmclone ? [tm-clone] :
);
! arg = DECL_ARGUMENTS (fndecl);
while (arg)
{
print_generic_expr (file, TREE_TYPE (arg), dump_flags);
*** dump_function_to_file (tree fn, FILE *fi
*** 6659,6689
fprintf (file, )\n);
if (flags TDF_VERBOSE)
! print_node (file, , fn, 2);
! dsf = DECL_STRUCT_FUNCTION (fn);
if (dsf (flags TDF_EH))
dump_eh_tree (file, dsf);
! if (flags TDF_RAW !gimple_has_body_p (fn))
{
! dump_node (fn, TDF_SLIM | flags, file);
return;
}
- /* Switch CFUN to point to FN. */
- push_cfun (DECL_STRUCT_FUNCTION (fn));
-
/* When GIMPLE is lowered, the variables are no longer available in
BIND_EXPRs, so display them separately. */
! if (cfun cfun-decl == fn (cfun-curr_properties PROP_gimple_lcf))
{
unsigned ix;
ignore_topmost_bind = true;
fprintf (file, {\n);
! if (!VEC_empty (tree, cfun-local_decls))
!
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Wed, Aug 15, 2012 at 05:21:04PM +0200, Martin Jambor 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 Yesterday I actually tried and it is not the only problem. Another one is dump_function_to_file-dump_bb-maybe_hot_bb_p which uses cfun to read profile_status. There may be others, this one just blew up first when I set cfun to NULL. And in future someone is quite likely to need cfun to dump something new too. At the same time, re-implementing dumping c-family/c-gimplify.c:c_genericize when dump_function suffices seems ugly to me. So I am going to declare dump_function a front-end interface and use set_cfun in my original patch in dump_function_to_file like we do in other such functions. I hope that will be OK. Thanks, Martin PS: Each of various alternatives proposed in this thread had someone who opposed it. If there is a consensus that some of them should be implemented anyway (like global value profiling hash), I am willing to do that, I just do not want to end up bickering about the result.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Tue, Aug 21, 2012 at 1:27 PM, Martin Jambor mjam...@suse.cz wrote: On Wed, Aug 15, 2012 at 05:21:04PM +0200, Martin Jambor 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 Yesterday I actually tried and it is not the only problem. Another one is dump_function_to_file-dump_bb-maybe_hot_bb_p which uses cfun to read profile_status. There may be others, this one just blew up first when I set cfun to NULL. And in future someone is quite likely to need cfun to dump something new too. At the same time, re-implementing dumping c-family/c-gimplify.c:c_genericize when dump_function suffices seems ugly to me. So I am going to declare dump_function a front-end interface and use set_cfun in my original patch in dump_function_to_file like we do in other such functions. I hope that will be OK. Thanks, Setting cfun has side-effects of switching target stuff which might have code-generation side-effects because of implementation issues we have with target/optimize attributes. So I don't think cfun should be changed just for dumping. Can you instead just set current_function_decl and access struct function via DECL_STRUCT_FUNCTION in the dumpers then? After all, it it is a front-end interface, the frontend way of saying this is the current function is to set current_function_decl, not the middle-end cfun. Richard. Martin PS: Each of various alternatives proposed in this thread had someone who opposed it. If there is a consensus that some of them should be implemented anyway (like global value profiling hash), I am willing to do that, I just do not want to end up bickering about the result.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Wed, Aug 15, 2012 at 5:21 PM, Martin Jambor mjam...@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
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
Hi, On Thu, 16 Aug 2012, Richard Guenther wrote: 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? Actually I must say, that none of the above three options appeal to me very much, and that the current solution (passing cfun via a global variable) is better than any of these: 1) passing it down in arguments: uglifies interface for a very special situation. 3) making the hash global is a layering violation in its own, and for instance would complicate a scheme where the memory for instructions (and their histograms) comes from per-function pools. 2) that's actually the most appealing one from a user interface, i.e. if there's a generic way of mapping gimple - FUNCTION_DECL. Certainly not at the cost of an additional pointer in each bb, but there are other schemes that we could use, for instance: Every function has an ENTRY and EXIT block. That one for certain is reachable via going bb-prev_bb until you hit the end. The entry block doesn't contain much interesting things, so we for instance can reuse, I don't know, the loop_father pointer to actually point to the function_decl containing it. Looking up the function from a bb would hence be a fairly expensive operation (walking the block chain), so it requires some care to use it, but I think that's okay. Ciao, Michael.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Thu, Aug 16, 2012 at 8:40 AM, Michael Matz m...@suse.de wrote: Hi, On Thu, 16 Aug 2012, Richard Guenther wrote: 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? Actually I must say, that none of the above three options appeal to me very much, and that the current solution (passing cfun via a global variable) is better than any of these: 1) passing it down in arguments: uglifies interface for a very special situation. 3) making the hash global is a layering violation in its own, and for instance would complicate a scheme where the memory for instructions (and their histograms) comes from per-function pools. 2) that's actually the most appealing one from a user interface, i.e. if there's a generic way of mapping gimple - FUNCTION_DECL. Certainly not at the cost of an additional pointer in each bb, but there are other schemes that we could use, for instance: Every function has an ENTRY and EXIT block. That one for certain is reachable via going bb-prev_bb until you hit the end. The entry block doesn't contain much interesting things, so we for instance can reuse, I don't know, the loop_father pointer to actually point to the function_decl containing it. Looking up the function from a bb would hence be a fairly expensive operation (walking the block chain), so it requires some care to use it, but I think that's okay. I like this approach, particularly since it would be used in contexts where there is no simpler scheme. I'm not crazy about overloading unused fields, but it's fine if we wrap it around a special accessor. I suppose we could also make ENTRY/EXIT be a base class for basic_block and use a real field (but that can wait). There are other pieces of data that are global but context-dependent. I wonder if it wouldn't make sense to encapsulate them in some class that is controlled by the pass manager and gets updated as it moves from function to function (it would contain things like cfun, current_function_decl, etc). Diego.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
Hi, On Thu, 16 Aug 2012, Diego Novillo wrote: I like this approach, particularly since it would be used in contexts where there is no simpler scheme. I'm not crazy about overloading unused fields, but it's fine if we wrap it around a special accessor. I suppose we could also make ENTRY/EXIT be a base class for basic_block and use a real field (but that can wait). Actually the other way around (ENTRY/EXIT be a subclass), as it would be an additional field to the normal basic blocks. But for central data structures I'm usually of the opinion that jumping through hoops just to not enlarge them is quite acceptable :) There are other pieces of data that are global but context-dependent. I wonder if it wouldn't make sense to encapsulate them in some class that is controlled by the pass manager and gets updated as it moves from function to function (it would contain things like cfun, current_function_decl, etc). And on that topic: I think it makes sense to eventually get rid of one or the other, current_function_decl is always (well, mostly) cfun-decl, if cfun happens to be set. I know there are some roadblocks in front of that goal, but still. Ciao, Michael.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Thu, Aug 16, 2012 at 10:21 AM, Michael Matz m...@suse.de wrote: Hi, On Thu, 16 Aug 2012, Diego Novillo wrote: I like this approach, particularly since it would be used in contexts where there is no simpler scheme. I'm not crazy about overloading unused fields, but it's fine if we wrap it around a special accessor. I suppose we could also make ENTRY/EXIT be a base class for basic_block and use a real field (but that can wait). Actually the other way around (ENTRY/EXIT be a subclass), as it would be Yeah, sorry. an additional field to the normal basic blocks. But for central data structures I'm usually of the opinion that jumping through hoops just to not enlarge them is quite acceptable :) Yeah, adding a full word when it's only used in a few places does not seem like a good tradeoff. There are other pieces of data that are global but context-dependent. I wonder if it wouldn't make sense to encapsulate them in some class that is controlled by the pass manager and gets updated as it moves from function to function (it would contain things like cfun, current_function_decl, etc). And on that topic: I think it makes sense to eventually get rid of one or the other, current_function_decl is always (well, mostly) cfun-decl, if cfun happens to be set. I know there are some roadblocks in front of that goal, but still. Agreed. Diego.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
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. Thanks, Martin
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
- 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. -- Eric Botcazou
[PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
Hi, I've always found it silly that in order to change the current function one has to call push_cfun and pop_cfun which conveniently set and restore the value of cfun and in addition to that also set current_function_decl and usually also cache its old value to restore it back afterwards. I also think that, at least throughout the middle-end, we should strive to have current_function_decl consistent with cfun-decl. There are quite a few places where we are not consistent and I think such situations are prone to nasty surprises as various functions rely on cfun and others on current_function_decl and it's easy to be unaware that one of the two is incorrect at the moment. This week I have therefore decided to try and make push_cfun, pop_cfun and push_struct_function also set the current_function_decl. Being afraid of opening a giant can of worms I only a mid-sized hole and left various set_cfuns for later as well as places where we set current_function_decl without bothering with cfun. After a few debugging sessions I came up with the patch below. The changes are mostly mechanical, let me try and explain some of the difficult or not-quite-nice ones, most of which come from calls from front-ends which generally do not care about cfun all that much. - In order to ensure that pop_cfun will reliable restore the old current_function_decl, push_cfun asserts that cfun and current_function_decl match. pop_cfun then simply restores current_function_decl to new_cfun-decl or NULL_TREE if new_cfun is NULL. To check that the two remain consistent, pop_cfun has a similar (albeit checking) assert. - I had to allow push_cfun(NULL) because in gfc_get_extern_function_decl in fortran/trans-decl.c we momentarily emulate top-level context by doing: current_function_decl = NULL_TREE; push_cfun (cfun); do_something () pop_cfun (); current_function_decl = save_fn_decl; and to keep current_function_decl consistent with cfun, cfun had to be made NULL too. Co I converted the above to push_cfun (NULL) which also sets current_function_decl to NULL_TREE. - I also had to allow push_cfun(NULL) because dwarf2out_abstract_function does just that, just it looks like: push_cfun (DECL_STRUCT_FUNCTION (decl)); but DECL_STRUCT_FUNCTION is usually (always?) NULL for abstract origin functions. But this also means that changed push_cfun sets current_function_decl to NULL, which means the abstract function is not dwarf2outed as it should be. Thus, in perhaps the most awful thunk in this patch I re-set current_function_decl after calling push_cfun. If someone has a better idea how to deal with this, I'm certainly interested. For the same reason I do not assert that current_function matches cfun-decl in pop_cfun if cfun is NULL. - each cfun change also triggers a pair of init_dummy_function_start and expand_dummy_function_end which invoke push_struct_function and pop_cfun respectively. Because we may be in the middle of another push/pop_cfun, the current_function_decl may not match and so the asserts are disabled in these cases, fortunately we can recognize them by looking at value of in_dummy_function. - 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. The patch passes bootstrap and testing on x86_64-linux (all languages + ada + obj-c++) and ia64-linux (c,c++,fortran,objc,obj-c++). There is some confusing jitter in the go testing results which I have not yet looked at (perhaps compare_tests just can't deal with it, there are tests reported both as newly failing and newly working etc...) but I thought that I'd send the patch now anyway to get some feedback in case I was doing something else wrong (I also do not know whether anyone but Ian can modify the go front-end). I have also LTO-built Mozilla Firefox with the patch. Well, what do you think? Martin 2012-08-08 Martin Jambor mjam...@suse.cz * function.c (push_cfun): Check old current_function_decl matches old cfun, set new current_function_decl to the decl of the new cfun. (push_struct_function): Likewise. (pop_cfun): Likewise. (allocate_struct_function): Move call to invoke_set_current_function_hook to the end of the function. * cfgexpand.c (estimated_stack_frame_size): Do not set and restore current_function_decl. * cgraph.c (cgraph_release_function_body): Likewise. * cgraphunit.c (cgraph_process_new_functions): Likewise.
Re: [PATCH] Set current_function_decl in {push,pop}_cfun and push_struct_function
On Thu, Aug 9, 2012 at 4:26 PM, Martin Jambor mjam...@suse.cz wrote: Hi, I've always found it silly that in order to change the current function one has to call push_cfun and pop_cfun which conveniently set and restore the value of cfun and in addition to that also set current_function_decl and usually also cache its old value to restore it back afterwards. I also think that, at least throughout the middle-end, we should strive to have current_function_decl consistent with cfun-decl. There are quite a few places where we are not consistent and I think such situations are prone to nasty surprises as various functions rely on cfun and others on current_function_decl and it's easy to be unaware that one of the two is incorrect at the moment. This week I have therefore decided to try and make push_cfun, pop_cfun and push_struct_function also set the current_function_decl. Being afraid of opening a giant can of worms I only a mid-sized hole and left various set_cfuns for later as well as places where we set current_function_decl without bothering with cfun. After a few debugging sessions I came up with the patch below. The changes are mostly mechanical, let me try and explain some of the difficult or not-quite-nice ones, most of which come from calls from front-ends which generally do not care about cfun all that much. - In order to ensure that pop_cfun will reliable restore the old current_function_decl, push_cfun asserts that cfun and current_function_decl match. pop_cfun then simply restores current_function_decl to new_cfun-decl or NULL_TREE if new_cfun is NULL. To check that the two remain consistent, pop_cfun has a similar (albeit checking) assert. - I had to allow push_cfun(NULL) because in gfc_get_extern_function_decl in fortran/trans-decl.c we momentarily emulate top-level context by doing: current_function_decl = NULL_TREE; push_cfun (cfun); do_something () pop_cfun (); current_function_decl = save_fn_decl; and to keep current_function_decl consistent with cfun, cfun had to be made NULL too. Co I converted the above to push_cfun (NULL) which also sets current_function_decl to NULL_TREE. - I also had to allow push_cfun(NULL) because dwarf2out_abstract_function does just that, just it looks like: push_cfun (DECL_STRUCT_FUNCTION (decl)); but DECL_STRUCT_FUNCTION is usually (always?) NULL for abstract origin functions. But this also means that changed push_cfun sets current_function_decl to NULL, which means the abstract function is not dwarf2outed as it should be. Thus, in perhaps the most awful thunk in this patch I re-set current_function_decl after calling push_cfun. If someone has a better idea how to deal with this, I'm certainly interested. For the same reason I do not assert that current_function matches cfun-decl in pop_cfun if cfun is NULL. - each cfun change also triggers a pair of init_dummy_function_start and expand_dummy_function_end which invoke push_struct_function and pop_cfun respectively. Because we may be in the middle of another push/pop_cfun, the current_function_decl may not match and so the asserts are disabled in these cases, fortunately we can recognize them by looking at value of in_dummy_function. - 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. The patch passes bootstrap and testing on x86_64-linux (all languages + ada + obj-c++) and ia64-linux (c,c++,fortran,objc,obj-c++). There is some confusing jitter in the go testing results which I have not yet looked at (perhaps compare_tests just can't deal with it, there are tests reported both as newly failing and newly working etc...) but I thought that I'd send the patch now anyway to get some feedback in case I was doing something else wrong (I also do not know whether anyone but Ian can modify the go front-end). I have also LTO-built Mozilla Firefox with the patch. Well, what do you think? Well. We should try to get rid of most push/pop_cfun calls, and the middle-end should never need to look at current_function_decl ... (in practice we have tree.c and fold-const.c which has to because its shared between FE and middle-end). For example the use in estimate_stack_frame_size. Or the uses in IPA passes. It would be nice to figure out which parts need access to cfun/current_function_decl in them (thus, arrange cfun/current_function_decl to be NULL there). Other than that, yes -
