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))
> ! FOR_EACH_LOCAL_DECL (cfun, ix, var)
> {
> print_generic_decl (file, var, flags);
> if (flags & TDF_VERBOSE)
> --- 6661,6688 ----
> fprintf (file, ")\n");
>
> if (flags & TDF_VERBOSE)
> ! print_node (file, "", fndecl, 2);
>
> ! dsf = DECL_STRUCT_FUNCTION (fndecl);
> if (dsf && (flags & TDF_EH))
> dump_eh_tree (file, dsf);
>
> ! if (flags & TDF_RAW && !gimple_has_body_p (fndecl))
> {
> ! dump_node (fndecl, TDF_SLIM | flags, file);
> return;
^^^^
and maybe other early exits from dump_function_to_file.
Also as said on IRC when you switch current_function_decl you should be
able to set cfun to NULL (and restore it afterwards, without invoking the
target hook for cfun switching). Thus, encapsulating this into a
push/pop_current_function_decl would be nice.
Thanks,
Richard.
