Don't dump low gimple functions in gimple dump

Don't dump low gimple functions in gimple dump

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

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

[-- Attachment #2: 0001-Don-t-dump-already-lowered-functions-in-gimple-dump.patch --]
[-- Type: text/x-patch, Size: 5713 bytes --]

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(-)
 create mode 100644 gcc/testsuite/gcc.dg/gomp/dump-task.c

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 9b51135..2ff4079 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -491,6 +491,20 @@ cgraph_add_new_function (tree fndecl, bool lowered)
 {
   gcc::pass_manager *passes = g->get_passes ();
   struct 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 (cgraph_state)
     {
       case CGRAPH_STATE_PARSING:
@@ -647,7 +661,6 @@ analyze_function (struct cgraph_node *node)
 	 body.  */
       if (!gimple_has_body_p (decl))
 	gimplify_function_tree (decl);
-      dump_function (TDI_generic, decl);
 
       /* Lower the function.  */
       if (!node->lowered)
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 3241633..065bf2c 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -59,6 +59,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "omp-low.h"
 #include "gimple-low.h"
 #include "cilk.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 */
@@ -8864,6 +8865,8 @@ gimplify_function_tree (tree fndecl)
   cfun->curr_properties = PROP_gimple_any;
 
   pop_cfun ();
+
+  dump_function (TDI_generic, fndecl);
 }
 
 /* Return a dummy expression of type TYPE in order to keep going after an
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index a2a64ad..0c497e9 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -71,6 +71,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "ipa-prop.h"
 #include "tree-nested.h"
 #include "tree-eh.h"
+#include "tree-dump.h"
 
 
 /* Lowering of OpenMP parallel and workshare constructs proceeds in two
@@ -1396,6 +1397,7 @@ finalize_task_copyfn (gimple task_stmt)
   pop_cfun ();
 
   /* Inform the callgraph about the new function.  */
+  dump_function (TDI_generic, child_fn);
   cgraph_add_new_function (child_fn, false);
 }
 
@@ -4843,6 +4845,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_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.  */
@@ -7963,6 +7967,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_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.  */
diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
index 8fa4dc8..368541c 100644
--- a/gcc/stor-layout.c
+++ b/gcc/stor-layout.c
@@ -297,7 +297,6 @@ finalize_size_functions (void)
       set_cfun (NULL);
       dump_function (TDI_original, fndecl);
       gimplify_function_tree (fndecl);
-      dump_function (TDI_generic, fndecl);
       cgraph_finalize_function (fndecl, false);
     }
 
diff --git a/gcc/testsuite/gcc.dg/gomp/dump-task.c b/gcc/testsuite/gcc.dg/gomp/dump-task.c
new file mode 100644
index 0000000..08ae5c3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gomp/dump-task.c
@@ -0,0 +1,33 @@
+/* https://gcc.gnu.org/ml/gcc/2014-03/msg00312.html
+   https://gcc.gnu.org/ml/gcc/2014-05/msg00117.html */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fopenmp -fdump-tree-gimple -fdump-tree-ompexp" } */
+
+int
+main(void)
+{
+#pragma omp parallel
+  {
+    extern void foo(void);
+    foo ();
+  }
+  return 0;
+}
+
+
+/* Check that low gimple representation does not end up in high gimple
+   dump.  */
+/* { dg-final { scan-tree-dump-not "main._omp_fn.0" "gimple" } } */
+
+/* Check for 3 references in ompexp dump: new function notification, function
+   dump and reference in main.  */
+/* { dg-final { scan-tree-dump-times "main._omp_fn.0" 3 "ompexp" } } */
+
+/* Check for function dump of main._omp_fn.0.  */
+/* { dg-final { scan-tree-dump-times "main._omp_fn.0 \\(void" 1 "ompexp" } } */
+
+/* Check for presence of function body of main._omp_fn.0.  */
+/* { dg-final { scan-tree-dump-times "foo \\(\\)" 1 "ompexp" } } */
+
+/* { dg-final { cleanup-tree-dump "gimple" } } */
+/* { dg-final { cleanup-tree-dump "ompexp" } } */
-- 
1.9.1

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

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 8076 bytes --]

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:
@@ -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);
 }
 
 /* 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);
   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);
 
 #ifdef ENABLE_OFFLOADING
       /* Add the new function to the offload table.  */
diff --git gcc/stor-layout.c gcc/stor-layout.c
index 273a12b..b3f9852 100644
--- gcc/stor-layout.c
+++ gcc/stor-layout.c
@@ -318,7 +318,6 @@ finalize_size_functions (void)
       set_cfun (NULL);
       dump_function (TDI_original, fndecl);
       gimplify_function_tree (fndecl);
-      dump_function (TDI_generic, fndecl);
       cgraph_node::finalize_function (fndecl, false);
     }
 
diff --git gcc/testsuite/gcc.dg/gomp/dump-task.c gcc/testsuite/gcc.dg/gomp/dump-task.c
new file mode 100644
index 0000000..08ae5c3
--- /dev/null
+++ gcc/testsuite/gcc.dg/gomp/dump-task.c
@@ -0,0 +1,33 @@
+/* https://gcc.gnu.org/ml/gcc/2014-03/msg00312.html
+   https://gcc.gnu.org/ml/gcc/2014-05/msg00117.html */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fopenmp -fdump-tree-gimple -fdump-tree-ompexp" } */
+
+int
+main(void)
+{
+#pragma omp parallel
+  {
+    extern void foo(void);
+    foo ();
+  }
+  return 0;
+}
+
+
+/* Check that low gimple representation does not end up in high gimple
+   dump.  */
+/* { dg-final { scan-tree-dump-not "main._omp_fn.0" "gimple" } } */
+
+/* Check for 3 references in ompexp dump: new function notification, function
+   dump and reference in main.  */
+/* { dg-final { scan-tree-dump-times "main._omp_fn.0" 3 "ompexp" } } */
+
+/* Check for function dump of main._omp_fn.0.  */
+/* { dg-final { scan-tree-dump-times "main._omp_fn.0 \\(void" 1 "ompexp" } } */
+
+/* Check for presence of function body of main._omp_fn.0.  */
+/* { dg-final { scan-tree-dump-times "foo \\(\\)" 1 "ompexp" } } */
+
+/* { dg-final { cleanup-tree-dump "gimple" } } */
+/* { dg-final { cleanup-tree-dump "ompexp" } } */


Grüße,
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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

[Richard Biener]

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:
> @@ -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?

>    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?

>  #ifdef ENABLE_OFFLOADING
>        /* Add the new function to the offload table.  */
> diff --git gcc/stor-layout.c gcc/stor-layout.c
> index 273a12b..b3f9852 100644
> --- gcc/stor-layout.c
> +++ gcc/stor-layout.c
> @@ -318,7 +318,6 @@ finalize_size_functions (void)
>        set_cfun (NULL);
>        dump_function (TDI_original, fndecl);
>        gimplify_function_tree (fndecl);
> -      dump_function (TDI_generic, fndecl);
>        cgraph_node::finalize_function (fndecl, false);
>      }
>
> diff --git gcc/testsuite/gcc.dg/gomp/dump-task.c gcc/testsuite/gcc.dg/gomp/dump-task.c
> new file mode 100644
> index 0000000..08ae5c3
> --- /dev/null
> +++ gcc/testsuite/gcc.dg/gomp/dump-task.c
> @@ -0,0 +1,33 @@
> +/* https://gcc.gnu.org/ml/gcc/2014-03/msg00312.html
> +   https://gcc.gnu.org/ml/gcc/2014-05/msg00117.html */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fopenmp -fdump-tree-gimple -fdump-tree-ompexp" } */
> +
> +int
> +main(void)
> +{
> +#pragma omp parallel
> +  {
> +    extern void foo(void);
> +    foo ();
> +  }
> +  return 0;
> +}
> +
> +
> +/* Check that low gimple representation does not end up in high gimple
> +   dump.  */
> +/* { dg-final { scan-tree-dump-not "main._omp_fn.0" "gimple" } } */
> +
> +/* Check for 3 references in ompexp dump: new function notification, function
> +   dump and reference in main.  */
> +/* { dg-final { scan-tree-dump-times "main._omp_fn.0" 3 "ompexp" } } */
> +
> +/* Check for function dump of main._omp_fn.0.  */
> +/* { dg-final { scan-tree-dump-times "main._omp_fn.0 \\(void" 1 "ompexp" } } */
> +
> +/* Check for presence of function body of main._omp_fn.0.  */
> +/* { dg-final { scan-tree-dump-times "foo \\(\\)" 1 "ompexp" } } */
> +
> +/* { dg-final { cleanup-tree-dump "gimple" } } */
> +/* { dg-final { cleanup-tree-dump "ompexp" } } */
>
>
> Grüße,
>  Thomas

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 8542 bytes --]

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)?

Thanks,
- Tom


[-- Attachment #2: 0002-Don-t-dump-low-gimple-functions-in-gimple-dump.patch --]
[-- Type: text/x-patch, Size: 4386 bytes --]

Don't dump low gimple functions in gimple dump

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

	(cgraph_node::analyze): Don't dump function to gimple dump file.
	* gimplify.c: Add tree-dump.h include.
	(gimplify_function_tree): Dump function to gimple dump file.
	* stor-layout.c (finalize_size_functions): Don't dump function to gimple
	dump file.

	* gcc.dg/gomp/dump-new-function-2.c: New test.
	* gcc.dg/gomp/dump-new-function-3.c: Same.
	* gcc.dg/gomp/dump-new-function.c: Same.
---
 gcc/cgraphunit.c                                |  1 -
 gcc/gimplify.c                                  |  3 +++
 gcc/stor-layout.c                               |  1 -
 gcc/testsuite/gcc.dg/gomp/dump-new-function-2.c | 20 ++++++++++++++++++++
 gcc/testsuite/gcc.dg/gomp/dump-new-function-3.c | 13 +++++++++++++
 gcc/testsuite/gcc.dg/gomp/dump-new-function.c   | 16 ++++++++++++++++
 6 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/gomp/dump-new-function-2.c
 create mode 100644 gcc/testsuite/gcc.dg/gomp/dump-new-function-3.c
 create mode 100644 gcc/testsuite/gcc.dg/gomp/dump-new-function.c

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 5a62223..ed79346 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -648,7 +648,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 a/gcc/gimplify.c b/gcc/gimplify.c
index 721afd1..eef851a 100644
--- a/gcc/gimplify.c
+++ b/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 */
@@ -9449,6 +9450,8 @@ gimplify_function_tree (tree fndecl)
   cfun->curr_properties |= PROP_gimple_any;
 
   pop_cfun ();
+
+  dump_function (TDI_generic, fndecl);
 }
 
 /* Return a dummy expression of type TYPE in order to keep going after an
diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
index 82997dd..ba65d1a 100644
--- a/gcc/stor-layout.c
+++ b/gcc/stor-layout.c
@@ -321,7 +321,6 @@ finalize_size_functions (void)
       set_cfun (NULL);
       dump_function (TDI_original, fndecl);
       gimplify_function_tree (fndecl);
-      dump_function (TDI_generic, fndecl);
       cgraph_node::finalize_function (fndecl, false);
     }
 
diff --git a/gcc/testsuite/gcc.dg/gomp/dump-new-function-2.c b/gcc/testsuite/gcc.dg/gomp/dump-new-function-2.c
new file mode 100644
index 0000000..627d067
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gomp/dump-new-function-2.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fopenmp -fdump-tree-gimple" } */
+
+void __attribute__((noinline))
+baz (int *p)
+{
+}
+
+void
+foo (void)
+{
+  int p[2];
+  p[0] = 1;
+  p[1] = 3;
+  #pragma omp task firstprivate (p)
+    baz (p);
+}
+
+/* Check that new function does not end up in gimple dump.  */
+/* { dg-final { scan-tree-dump-not "foo\\._omp_cpyfn\\.1 \\(struct" "gimple" } } */
diff --git a/gcc/testsuite/gcc.dg/gomp/dump-new-function-3.c b/gcc/testsuite/gcc.dg/gomp/dump-new-function-3.c
new file mode 100644
index 0000000..1854179
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gomp/dump-new-function-3.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -ftree-parallelize-loops=2 -fdump-tree-gimple" } */
+
+void
+foo (int *__restrict a, int *__restrict b, int *__restrict c)
+{
+  int i;
+  for (i = 0; i < 1000; ++i)
+    c[i] = a[i] + b[i];
+}
+
+/* Check that new function does not end up in gimple dump.  */
+/* { dg-final { scan-tree-dump-not "foo\\._loopfn\\.0" "gimple" } } */
diff --git a/gcc/testsuite/gcc.dg/gomp/dump-new-function.c b/gcc/testsuite/gcc.dg/gomp/dump-new-function.c
new file mode 100644
index 0000000..2814260
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gomp/dump-new-function.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fopenmp -fdump-tree-gimple" } */
+
+int
+main (void)
+{
+#pragma omp parallel
+  {
+    extern void foo (void);
+    foo ();
+  }
+  return 0;
+}
+
+/* Check that new function does not end up in gimple dump.  */
+/* { dg-final { scan-tree-dump-not "main\\._omp_fn\\.0" "gimple" } } */
-- 
1.9.1

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

[Richard Biener]

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
>

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

[Jakub Jelinek]

On Thu, Jun 04, 2015 at 05:02:42PM +0200, Tom de Vries wrote:
> >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've committed following patch to gomp-4_5-branch for this.

The state before the patch is:
1) the omp_fn children created during the pre-SSA ompexp pass are dumped
   first in the *.ssa dump and in all the following ones (these are created
   as low gimple, non-SSA)
2) the omp_cpyfn children created during the pre-SSA ompexp pass are dumped
   first in the *.omplower dump and in all the following ones (these are
   created as high gimple)
3) the omp_fn children created during the parloops/ompexpssa passes
   are dumped first post-IPA (or during IPA?) and in all the following ones
   (these are created as SSA gimple)

2) and 3) is fine, on 1) I don't like the fact that one can see the child
functions only in the SSA form, not before it.  Thus, following patch
arranges for only the 1) case to change, those functions are dumped now
into the *.ompexp dump.  As e.g. for C++ sometimes the dumped function name
is not too descriptive (e.g. <built-in>), I've hacked things up so that also
the
;; Function
headers are dumped including the assembler name, and because there can be
many child functions emitted for a single original function, I chose to
dump again the
;; Function
header for the original function if any child functions were dumped.
The original function will have then two headers in the dump file, but I
think it is more readable that way.

2015-10-22  Jakub Jelinek  <jakub@redhat.com>

	* omp-low.c: Include tree-pretty-print.h.
	(omp_any_child_fn_dumped): New variable.
	(expand_omp_taskreg, expand_omp_target): Call
	assign_assembler_name_if_neeeded on child_fn if current_function_decl
	has assembler name set, but child_fn does not.  Dump the header
	and IL of the child function when not in SSA form.
	(expand_omp): Clear omp_any_child_fn_dumped.  Dump function header
	again if we have dumped any child functions.

--- gcc/omp-low.c.jj	2015-10-22 13:41:23.279881315 +0200
+++ gcc/omp-low.c	2015-10-22 14:11:23.488003543 +0200
@@ -81,6 +81,7 @@ along with GCC; see the file COPYING3.
 #include "context.h"
 #include "lto-section-names.h"
 #include "gomp-constants.h"
+#include "tree-pretty-print.h"
 
 /* Lowering of OMP parallel and workshare constructs proceeds in two
    phases.  The first phase scans the function looking for OMP statements
@@ -244,6 +245,7 @@ static int target_nesting_level;
 static struct omp_region *root_omp_region;
 static bitmap task_shared_vars;
 static vec<omp_context *> taskreg_contexts;
+static bool omp_any_child_fn_dumped;
 
 static void scan_omp (gimple_seq *, omp_context *);
 static tree scan_omp_1_op (tree *, int *, void *);
@@ -6739,9 +6741,15 @@ expand_omp_taskreg (struct omp_region *r
       node->parallelized_function = 1;
       cgraph_node::add_new_function (child_fn, true);
 
+      bool need_asm = DECL_ASSEMBLER_NAME_SET_P (current_function_decl)
+		      && !DECL_ASSEMBLER_NAME_SET_P (child_fn);
+
       /* Fix the callgraph edges for child_cfun.  Those for cfun will be
 	 fixed in a following pass.  */
       push_cfun (child_cfun);
+      if (need_asm)
+	assign_assembler_name_if_neeeded (child_fn);
+
       if (optimize)
 	optimize_omp_library_calls (entry_stmt);
       cgraph_edge::rebuild_edges ();
@@ -6767,6 +6775,13 @@ expand_omp_taskreg (struct omp_region *r
 	verify_loop_structure ();
 #endif
       pop_cfun ();
+
+      if (dump_file && !gimple_in_ssa_p (cfun))
+	{
+	  omp_any_child_fn_dumped = true;
+	  dump_function_header (dump_file, child_fn, dump_flags);
+	  dump_function_to_file (child_fn, dump_file, dump_flags);
+	}
     }
 
   /* Emit a library call to launch the children threads.  */
@@ -11575,9 +11590,14 @@ expand_omp_target (struct omp_region *re
       vec_safe_push (offload_funcs, child_fn);
 #endif
 
+      bool need_asm = DECL_ASSEMBLER_NAME_SET_P (current_function_decl)
+		      && !DECL_ASSEMBLER_NAME_SET_P (child_fn);
+
       /* Fix the callgraph edges for child_cfun.  Those for cfun will be
 	 fixed in a following pass.  */
       push_cfun (child_cfun);
+      if (need_asm)
+	assign_assembler_name_if_neeeded (child_fn);
       cgraph_edge::rebuild_edges ();
 
 #ifdef ENABLE_OFFLOADING
@@ -11605,6 +11625,13 @@ expand_omp_target (struct omp_region *re
 	verify_loop_structure ();
 #endif
       pop_cfun ();
+
+      if (dump_file && !gimple_in_ssa_p (cfun))
+	{
+	  omp_any_child_fn_dumped = true;
+	  dump_function_header (dump_file, child_fn, dump_flags);
+	  dump_function_to_file (child_fn, dump_file, dump_flags);
+	}
     }
 
   /* Emit a library call to launch the offloading region, or do data
@@ -11892,6 +11919,7 @@ expand_omp_target (struct omp_region *re
 static void
 expand_omp (struct omp_region *region)
 {
+  omp_any_child_fn_dumped = false;
   while (region)
     {
       location_t saved_location;
@@ -11975,6 +12003,12 @@ expand_omp (struct omp_region *region)
       input_location = saved_location;
       region = region->next;
     }
+  if (omp_any_child_fn_dumped)
+    {
+      if (dump_file)
+	dump_function_header (dump_file, current_function_decl, dump_flags);
+      omp_any_child_fn_dumped = false;
+    }
 }
 
 

	Jakub

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

[Tom de Vries]

On 22/10/15 14:27, Jakub Jelinek wrote:
> The state before the patch is:
> 1) the omp_fn children created during the pre-SSA ompexp pass are dumped
>     first in the *.ssa dump and in all the following ones (these are created
>     as low gimple, non-SSA)

Hi,

I do see those child fns before the ssa dump, in the fixup_cfg1 dump 
(and with -fipa-tree-all, also in the visibility dump, just after the 
ompexp dump).

[ Not that I disagree with dumping the child fn in ompexp dump. ]

> 2) the omp_cpyfn children created during the pre-SSA ompexp pass are dumped
>     first in the *.omplower dump and in all the following ones (these are
>     created as high gimple)
> 3) the omp_fn children created during the parloops/ompexpssa passes
>     are dumped first post-IPA (or during IPA?) and in all the following ones
>     (these are created as SSA gimple)
>

> 3) is fine

Not fine for me though.

As I wrote earlier ( 
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01316.html ):
...
The problems I'm trying to solve with this patch are the following:
- even if you're compiling a single function, part of the function
   occurs twice (once in the original, once in the split-off
   function), making formulating scan-tree-dumps more complicated for
   parloops.
- the intuitive thing is to look in the parloops tree-dump and look at
   the original function and the split-off function, and think that the
   split-off function is the immediate result of the split-off that
   happened in the pass, which is incorrect (since it jumps back in the
   pass list and traverses other passes before arriving back at
   parloops).

Adding something like a flag -fno-dump-new-function would solve the 
first problem, but not the second.

We could also dump new functions in a different dump file 
src.c.new.123t.pass, and this would solve both problems. But it will 
cause the 'where did that function go' confusion, certainly initially.
...

Thanks,
- Tom

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

[Jakub Jelinek]

On Thu, Oct 22, 2015 at 02:51:34PM +0200, Tom de Vries wrote:
> On 22/10/15 14:27, Jakub Jelinek wrote:
> >The state before the patch is:
> >1) the omp_fn children created during the pre-SSA ompexp pass are dumped
> >    first in the *.ssa dump and in all the following ones (these are created
> >    as low gimple, non-SSA)
> 
> Hi,
> 
> I do see those child fns before the ssa dump, in the fixup_cfg1 dump (and
> with -fipa-tree-all, also in the visibility dump, just after the ompexp
> dump).

Oh, indeed.  Any case, I think, with the patch 1) behaves the most desirable
way now, no dumps before ompexp show it, and ompexp dump shows both the new
child functions and original function with the outlined code already
removed.  And all dumps after ompexp show both as well.

> [ Not that I disagree with dumping the child fn in ompexp dump. ]
> 
> >2) the omp_cpyfn children created during the pre-SSA ompexp pass are dumped
> >    first in the *.omplower dump and in all the following ones (these are
> >    created as high gimple)

And this is probably fine too, as the cpyfn functions are artificially
created, they don't contain code that has been moved from somewhere else.

> >3) the omp_fn children created during the parloops/ompexpssa passes
> >    are dumped first post-IPA (or during IPA?) and in all the following ones
> >    (these are created as SSA gimple)

Bet what you'd want most would be to be able to create new functions and
mark them not just as SSA low gimple, but also be able to say, skip
everything in the pass pipeline for these functions until pass this and
this, i.e. start pass pipeline with pass->next of the creating pass.
Then it would neither show up in the earlier dumps, nor be processed
multiple times by the same passes.  We'd need the && !gimple_in_ssa_p
I've added in the patch removed once that is done...

	Jakub