| Message ID | 55706892.8050701@mentor.com |
|---|---|
| State | New |
| Headers | show |
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 >
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