Re: Don't dump low gimple functions in gimple dump

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

On Thu, Jun 4, 2015 at 5:02 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 22/05/15 11:27, Richard Biener wrote:
>>
>> On Thu, May 21, 2015 at 5:36 PM, Thomas Schwinge
>> <thomas@codesourcery.com> wrote:
>>>
>>> Hi!
>>>
>>> It's just been a year.  ;-P
>>>
>>> In early March, I (hopefully correctly) adapted Tom's patch to apply to
>>> then-current GCC trunk sources; posting this here.  Is the general
>>> approach OK?
>>>
>>> On Tue, 20 May 2014 10:16:45 +0200, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> Honza,
>>>>
>>>> Consider this program:
>>>> ...
>>>> int
>>>> main(void)
>>>> {
>>>> #pragma omp parallel
>>>>    {
>>>>      extern void foo(void);
>>>>      foo ();
>>>>    }
>>>>    return 0;
>>>> }
>>>> ...
>>>>
>>>> When compiling this program with -fopenmp, the ompexp pass splits off a
>>>> new
>>>> function called main._omp_fn.0 containing the call to foo.  The new
>>>> function is
>>>> then dumped into the gimple dump by analyze_function.
>>>>
>>>> There are two problems with this:
>>>> - the new function is in low gimple, and is dumped next to high gimple
>>>>    functions
>>>> - since it's already low, the new function is not lowered, and 'goes
>>>> missing'
>>>>    in the dumps following the gimple dump, until it reappears again
>>>> after the
>>>>    last lowering dump.
>>>>    [ http://gcc.gnu.org/ml/gcc/2014-03/msg00312.html ]
>>>>
>>>> This patch fixes the problems by ensuring that analyze_function only
>>>> dumps the
>>>> new function to the gimple dump after gimplification (specifically, by
>>>> moving
>>>> the dump_function call into gimplify_function_tree.  That makes the call
>>>> to
>>>> dump_function in finalize_size_functions superfluous).
>>>>
>>>> That also requires us to add a call to dump_function in
>>>> finalize_task_copyfn,
>>>> where we split off a new high gimple function.
>>>>
>>>> And in expand_omp_taskreg and expand_omp_target, where we split off a
>>>> new low
>>>> gimple function, we now dump the new function into the current (ompexp)
>>>> dump
>>>> file, which is the last lowering dump.
>>>>
>>>> Finally, we dump an information statement at the start of
>>>> cgraph_add_new_function to give a better idea when and what kind of
>>>> function is
>>>> created.
>>>>
>>>> Bootstrapped and reg-tested on x86_64.
>>>>
>>>> OK for trunk ?
>>>>
>>>> Thanks,
>>>> - Tom
>>>
>>>
>>> commit b925b393c3d975a9281789d97aff8a91a8b53be0
>>> Author: Thomas Schwinge <thomas@codesourcery.com>
>>> Date:   Sun Mar 1 15:05:15 2015 +0100
>>>
>>>      Don't dump low gimple functions in gimple dump
>>>
>>>      id:"537B0F6D.7060808@mentor.com" or id:"53734DC5.90001@mentor.com"
>>>
>>>      2014-05-19  Tom de Vries  <tom@codesourcery.com>
>>>
>>>          * cgraphunit.c (cgraph_add_new_function): Dump message on new
>>> function.
>>>          (analyze_function): Don't dump function to gimple dump file.
>>>          * gimplify.c: Add tree-dump.h include.
>>>          (gimplify_function_tree): Dump function to gimple dump file.
>>>          * omp-low.c: Add tree-dump.h include.
>>>          (finalize_task_copyfn): Dump new function to gimple dump file.
>>>          (expand_omp_taskreg, expand_omp_target): Dump new function to
>>> dump file.
>>>          * stor-layout.c (finalize_size_functions): Don't dump function
>>> to gimple
>>>          dump file.
>>>
>>>          * gcc.dg/gomp/dump-task.c: New test.
>>> ---
>>>   gcc/cgraphunit.c                      | 15 ++++++++++++++-
>>>   gcc/gimplify.c                        |  3 +++
>>>   gcc/omp-low.c                         |  6 ++++++
>>>   gcc/stor-layout.c                     |  1 -
>>>   gcc/testsuite/gcc.dg/gomp/dump-task.c | 33
>>> +++++++++++++++++++++++++++++++++
>>>   5 files changed, 56 insertions(+), 2 deletions(-)
>>>
>>> diff --git gcc/cgraphunit.c gcc/cgraphunit.c
>>> index 8280fc4..0860c86 100644
>>> --- gcc/cgraphunit.c
>>> +++ gcc/cgraphunit.c
>>> @@ -501,6 +501,20 @@ cgraph_node::add_new_function (tree fndecl, bool
>>> lowered)
>>>   {
>>>     gcc::pass_manager *passes = g->get_passes ();
>>>     cgraph_node *node;
>>> +
>>> +  if (dump_file)
>>> +    {
>>> +      const char *function_type = ((gimple_has_body_p (fndecl))
>>> +                                  ? (lowered
>>> +                                     ? "low gimple"
>>> +                                     : "high gimple")
>>> +                                  : "to-be-gimplified");
>>> +      fprintf (dump_file,
>>> +              "Added new %s function %s to callgraph\n",
>>> +              function_type,
>>> +              fndecl_name (fndecl));
>>> +    }
>>> +
>>>     switch (symtab->state)
>>>       {
>>>         case PARSING:
>
>
> Split off this hunk as a seperate patch:
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00416.html .
>
>
>>> @@ -629,7 +643,6 @@ cgraph_node::analyze (void)
>>>           body.  */
>>>         if (!gimple_has_body_p (decl))
>>>          gimplify_function_tree (decl);
>>> -      dump_function (TDI_generic, decl);
>>>
>>>         /* Lower the function.  */
>>>         if (!lowered)
>>> diff --git gcc/gimplify.c gcc/gimplify.c
>>> index 9214648..d6c500d 100644
>>> --- gcc/gimplify.c
>>> +++ gcc/gimplify.c
>>> @@ -87,6 +87,7 @@ along with GCC; see the file COPYING3.  If not see
>>>   #include "gimple-low.h"
>>>   #include "cilk.h"
>>>   #include "gomp-constants.h"
>>> +#include "tree-dump.h"
>>>
>>>   #include "langhooks-def.h"     /* FIXME: for
>>> lhd_set_decl_assembler_name */
>>>   #include "tree-pass.h"         /* FIXME: only for PROP_gimple_any */
>>> @@ -9435,6 +9436,8 @@ gimplify_function_tree (tree fndecl)
>>>     cfun->curr_properties = PROP_gimple_any;
>>>
>>>     pop_cfun ();
>>> +
>>> +  dump_function (TDI_generic, fndecl);
>>
>>
>> Err - that dumps gimple now.  I think the dump you removed above was
>> simply bogus
>> and should have used TDI_gimple?  Or is TDI_generic just misnamed?
>>
>> Ah, indeed.  There is TDI_original (for .original, aka GENERIC) and
>> TDI_generic
>> for .gimple.
>>
>>>   }
>>>
>>>   /* Return a dummy expression of type TYPE in order to keep going after
>>> an
>>> diff --git gcc/omp-low.c gcc/omp-low.c
>>> index fac32b3..2839d8f 100644
>>> --- gcc/omp-low.c
>>> +++ gcc/omp-low.c
>>> @@ -109,6 +109,7 @@ along with GCC; see the file COPYING3.  If not see
>>>   #include "lto-section-names.h"
>>>   #include "gomp-constants.h"
>>>   #include "gimple-pretty-print.h"
>>> +#include "tree-dump.h"
>>>
>>>
>>>   /* Lowering of OMP parallel and workshare constructs proceeds in two
>>> @@ -1714,6 +1715,7 @@ finalize_task_copyfn (gomp_task *task_stmt)
>>>     pop_cfun ();
>>>
>>>     /* Inform the callgraph about the new function.  */
>>> +  dump_function (TDI_generic, child_fn);
>>
>>
>> And this would be duplicate?
>>
>
> No, it would be dumped here in the gimple dump. Nothing else would dump it
> in the gimple dump, so there would be no duplicate.
>
>>>     cgraph_node::add_new_function (child_fn, false);
>>>   }
>>>
>>> @@ -6073,6 +6075,8 @@ expand_omp_taskreg (struct omp_region *region)
>>>         /* Inform the callgraph about the new function.  */
>>>         DECL_STRUCT_FUNCTION (child_fn)->curr_properties =
>>> cfun->curr_properties;
>>>         cgraph_node::add_new_function (child_fn, true);
>>> +      if (dump_file)
>>> +       dump_function_to_file (child_fn, dump_file, dump_flags);
>>>
>>>         /* Fix the callgraph edges for child_cfun.  Those for cfun will
>>> be
>>>           fixed in a following pass.  */
>>> @@ -9527,6 +9531,8 @@ expand_omp_target (struct omp_region *region)
>>>         /* Inform the callgraph about the new function.  */
>>>         DECL_STRUCT_FUNCTION (child_fn)->curr_properties =
>>> cfun->curr_properties;
>>>         cgraph_node::add_new_function (child_fn, true);
>>> +      if (dump_file)
>>> +       dump_function_to_file (child_fn, dump_file, dump_flags);
>>
>>
>> So why does add_new_function not do the dumping and to the correct
>> place?  That is,
>> you are dumping things twice here, once in omp expansion and then again
>> when the
>> new function reaches omp expansion?
>>
>
> Dumping twice doesn't happen for omp-annotated source code. But indeed,
> dumping twice does happen in ompexpssa for the parloops/ompexpssa case with
> this patch, which is confusing.  And even if we would eliminate the dumping
> when the new function reaches omp expansion, still it would be confusing
> because the dump of the function would not be the version inbetween the
> preceding and succeeding dump.
>
> I'm submitting a simplified patch now, which focuses on reserving the gimple
> dump for the result of gimplification, without trying to dump the split-off
> function immediately after split-off.
>
> [ In principle, having the split-off function immediately after split-off is
> valuable. But the question is where to dump it, without causing confusion. ]
>
> Added test-cases for the task_copyfn and the parloops/ompexpssa cases.
>
> OK for trunk (once retested passes)?

2014-06-04  Tom de Vries  <tom@codesourcery.com>

        (cgraph_node::analyze): Don't dump function to gimple dump file.

changed file is missing.

Ok with that fixed.

Thanks,
Richard.

> Thanks,
> - Tom
>