From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 33E10385843E for ; Thu, 31 Mar 2022 12:38:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 33E10385843E Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 6021B281F62; Thu, 31 Mar 2022 14:38:18 +0200 (CEST) Date: Thu, 31 Mar 2022 14:38:18 +0200 From: Jan Hubicka To: Martin Jambor Cc: GCC Patches Subject: Re: [PATCH] ipa: Create LOAD references when necessary during inlining (PR 103171) Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Mar 2022 12:38:21 -0000 > Hi, > > in r12-2523-g13586172d0b70c ipa-prop tracking of jump functions during > inlining got the ability to remove ADDR references when inlining > discovered that they were not necessary or turn them into LOAD > references when we know that what was a function call argument passed > by reference will end up as a load (one or more). > > Unfortunately, the code only creates the LOAD references when > replacing removed ADDR references and PR 103171 showed that with some > ordering of inlining, we need to add the LOAD reference before we know > we can remove the ADDR one - or the reference will be lost, leading to > link errors or even ICEs. > > Specifically in testcase gcc.dg/lto/pr103171_1.c added in this patch, > if foo() is inlined to entry(), we need to create the LOAD reference > so that when later bar() is inlined into foo() and we discover that > the paameter is unused, we can remove the ADDR reference and still > keep the varaible around for the load. > > Bootstrapped, LTO bootstrapped and tested on x86_64-linux. OK for > trunk? OK, Thanks! Honza > > Thanks, > > Martin > > > > gcc/ChangeLog: > > 2022-01-28 Martin Jambor > > PR ipa/103171 > * ipa-prop.cc (propagate_controlled_uses): Add a LOAD reference > always when an ADDR_EXPR constant is known to reach a load because > of inlining, not just when removing an ADDR reference. > > gcc/testsuite/ChangeLog: > > 2022-01-28 Martin Jambor > > PR ipa/103171 > * gcc.dg/ipa/remref-6.c: Adjust dump scan string. > * gcc.dg/ipa/remref-7.c: New test. > * gcc.dg/lto/pr103171_0.c: New test. > * gcc.dg/lto/pr103171_1.c: Likewise. > --- > gcc/ipa-prop.cc | 30 ++++++++++++----------- > gcc/testsuite/gcc.dg/ipa/remref-6.c | 2 +- > gcc/testsuite/gcc.dg/ipa/remref-7.c | 33 +++++++++++++++++++++++++ > gcc/testsuite/gcc.dg/lto/pr103171_0.c | 11 +++++++++ > gcc/testsuite/gcc.dg/lto/pr103171_1.c | 35 +++++++++++++++++++++++++++ > 5 files changed, 96 insertions(+), 15 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/remref-7.c > create mode 100644 gcc/testsuite/gcc.dg/lto/pr103171_0.c > create mode 100644 gcc/testsuite/gcc.dg/lto/pr103171_1.c > > diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc > index e55fe2776f2..72aa3e2f60d 100644 > --- a/gcc/ipa-prop.cc > +++ b/gcc/ipa-prop.cc > @@ -4181,6 +4181,20 @@ propagate_controlled_uses (struct cgraph_edge *cs) > int d = ipa_get_controlled_uses (old_root_info, i); > int c = rdesc->refcount; > rdesc->refcount = combine_controlled_uses_counters (c, d); > + if (rdesc->refcount != IPA_UNDESCRIBED_USE > + && ipa_get_param_load_dereferenced (old_root_info, i)) > + { > + tree cst = ipa_get_jf_constant (jf); > + gcc_checking_assert (TREE_CODE (cst) == ADDR_EXPR > + && (TREE_CODE (TREE_OPERAND (cst, 0)) > + == VAR_DECL)); > + symtab_node *n = symtab_node::get (TREE_OPERAND (cst, 0)); > + new_root->create_reference (n, IPA_REF_LOAD, NULL); > + if (dump_file) > + fprintf (dump_file, "ipa-prop: Address IPA constant will reach " > + "a load so adding LOAD reference from %s to %s.\n", > + new_root->dump_name (), n->dump_name ()); > + } > if (rdesc->refcount == 0) > { > tree cst = ipa_get_jf_constant (jf); > @@ -4193,20 +4207,8 @@ propagate_controlled_uses (struct cgraph_edge *cs) > symtab_node *n = symtab_node::get (TREE_OPERAND (cst, 0)); > if (n) > { > - struct cgraph_node *clone; > - bool removed = remove_described_reference (n, rdesc); > - /* The reference might have been removed by IPA-CP. */ > - if (removed > - && ipa_get_param_load_dereferenced (old_root_info, i)) > - { > - new_root->create_reference (n, IPA_REF_LOAD, NULL); > - if (dump_file) > - fprintf (dump_file, "ipa-prop: ...replaced it with " > - "LOAD one from %s to %s.\n", > - new_root->dump_name (), n->dump_name ()); > - } > - > - clone = cs->caller; > + remove_described_reference (n, rdesc); > + cgraph_node *clone = cs->caller; > while (clone->inlined_to > && clone->ipcp_clone > && clone != rdesc->cs->caller) > diff --git a/gcc/testsuite/gcc.dg/ipa/remref-6.c b/gcc/testsuite/gcc.dg/ipa/remref-6.c > index 7deae3114a4..f31f4c14319 100644 > --- a/gcc/testsuite/gcc.dg/ipa/remref-6.c > +++ b/gcc/testsuite/gcc.dg/ipa/remref-6.c > @@ -20,5 +20,5 @@ void entry() > } > > /* { dg-final { scan-ipa-dump "Removed a reference" "inline" } } */ > -/* { dg-final { scan-ipa-dump "replaced it with LOAD" "inline" } } */ > +/* { dg-final { scan-ipa-dump "adding LOAD reference" "inline" } } */ > /* { dg-final { scan-tree-dump-not "builtin_exp" "optimized" } } */ > diff --git a/gcc/testsuite/gcc.dg/ipa/remref-7.c b/gcc/testsuite/gcc.dg/ipa/remref-7.c > new file mode 100644 > index 00000000000..b2c26ab7fd5 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/remref-7.c > @@ -0,0 +1,33 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fno-early-inlining -fno-ipa-sra -fdump-ipa-inline" } */ > + > +int rglobal = 0; > +int g; > + > +int c; > +double *array; > + > +/* unused parameter */ > +static void bar(int *p) > +{ > + int i; > + for (i = 0; i < c; i++) > + { > + /* something big so that it is inlined second. */ > + array[i] = __builtin_exp(array[i]+1)*2; > + } > +} > + > +void foo(int *p) { > + g = *p; > + bar(p); > +} > + > +void entry() > +{ > + foo(&rglobal); > +} > + > +/* { dg-final { scan-ipa-dump "Removed a reference" "inline" } } */ > +/* { dg-final { scan-ipa-dump "adding LOAD reference" "inline" } } */ > + > diff --git a/gcc/testsuite/gcc.dg/lto/pr103171_0.c b/gcc/testsuite/gcc.dg/lto/pr103171_0.c > new file mode 100644 > index 00000000000..5dc17d6c6c8 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/pr103171_0.c > @@ -0,0 +1,11 @@ > +/* { dg-lto-do link } */ > +/* { dg-lto-options { { -O2 -flto -flto-partition=1to1 -fno-early-inlining -fno-ipa-sra -w } } } */ > + > +extern void __attribute__((noinline)) entry(void); > + > +int > +main (int argc, char **argv) > +{ > + entry(); > + return 0; > +} > diff --git a/gcc/testsuite/gcc.dg/lto/pr103171_1.c b/gcc/testsuite/gcc.dg/lto/pr103171_1.c > new file mode 100644 > index 00000000000..39aed25daf7 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/pr103171_1.c > @@ -0,0 +1,35 @@ > +int rglobal = 0; > + > +volatile int g; > +volatile int c; > +volatile double *array; > + > +/* unused parameter */ > +static void > +bar(int *p) > +{ > + int i; > + for (i = 0; i < c; i++) > + { > + /* something big so that it is inlined second. */ > + array[i] = (array[i+1]+array[i]+1)*2; > + } > +} > + > +void foo(int *p) { > + g = *p; > + bar(p); > +} > + > +void __attribute__((noinline)) > +entry(void) > +{ > + foo(&rglobal); > +} > + > +void __attribute__((used)) > +blah(int *p) > +{ > + bar(p); > +} > + > -- > 2.34.1 >