From patchwork Thu Jun 4 15:02:42 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 480758 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 71F98140157 for ; Fri, 5 Jun 2015 01:03:04 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=p36ydIFY; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=dQQFMXG3wIWmE82CZ OmfXr+gG29JnWv88g0ReKBvlwslP1IVsnVzCFQl6b8mG228cL++VWoDvkB7w2zgY jCHt6rPmH5lE3CH4O4TpxafnT+OjKvFQ4JYhqXSVkG3LGWXt6XwPpRCx5lRsXRg/ v2cwIAsR7XZ3TyxS60kBh4qtZI= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=rDzKQF/NR4YAuU5ladPXC5s gAsE=; b=p36ydIFYgINKgpcc2Nl/hHNUQtRTI2Yy4LTnckjUu6clLgZtLf62Q2d cpPsPR8Q9IR2THB1i/mqIipAw3VWkabbjCZ4iuF9wHZzdF3KoySqZfilDtnkqehg LgBl60wvBRbFpRIFHgqoBxvXVPqpZoPjxt7LpYN7oOXRFathuBgY= Received: (qmail 67750 invoked by alias); 4 Jun 2015 15:02:58 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 67735 invoked by uid 89); 4 Jun 2015 15:02:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_05, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 04 Jun 2015 15:02:55 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1Z0WfS-00048N-FJ from Tom_deVries@mentor.com ; Thu, 04 Jun 2015 08:02:51 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Thu, 4 Jun 2015 16:02:48 +0100 Message-ID: <55706892.8050701@mentor.com> Date: Thu, 4 Jun 2015 17:02:42 +0200 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Richard Biener , Thomas Schwinge CC: Jan Hubicka , "gcc-patches@gcc.gnu.org" , Jakub Jelinek , Julian Brown Subject: Re: Don't dump low gimple functions in gimple dump References: <87k3bnecq3.fsf@schwinge.name> <537B0F6D.7060808@mentor.com> <87617mndrv.fsf@kepler.schwinge.homeip.net> In-Reply-To: On 22/05/15 11:27, Richard Biener wrote: > On Thu, May 21, 2015 at 5:36 PM, Thomas Schwinge > 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 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 >> 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 >> >> * 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 Don't dump low gimple functions in gimple dump 2014-06-04 Tom de Vries (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