From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21606 invoked by alias); 20 Jul 2018 02:49:05 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 21377 invoked by uid 89); 20 Jul 2018 02:48:37 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS,TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=excessive, sk:scan-ip, sk:scanip, strcpy X-HELO: aserp2130.oracle.com Received: from aserp2130.oracle.com (HELO aserp2130.oracle.com) (141.146.126.79) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 20 Jul 2018 02:48:19 +0000 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w6K2hUHF015121; Fri, 20 Jul 2018 02:48:16 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : from : to : cc : references : message-id : date : mime-version : in-reply-to : content-type; s=corp-2018-07-02; bh=ljHTl7ImdAsRrsYAPUrJhU3R0Ilb75fiUNFdQTPe1ls=; b=YV+57XMdSZg/VwxBP5vT5yndXi/fmPwc+Xw3X/WhQNf9kA3aXLKQl3ZziXkMsIYIo3J7 gWpeUf7CBj8M89TtHB5vc+SOe3aqoY6Gj6pPfWtP27gkdQefBr53SAedV/PejSfap6Fd +LpbRBTJQcL9+TLAxUEWu6ZwcjwZHLDok2fooo0JVmKqF0OoSgAMjxOIwYRE5Awa7hk1 sgHO9IKMXGHVXc1j9qdAT4+5VENBvTs3l+zHoNcAQH/LDwFMEHL5eF3Zqb8gXk5r2l/T SXODXIaS1TdYgrQQJQBsSF+BAyeQkQhvpiPA2loTL3b3QXgxapI2kspQ5vSgCe5bIWFD 4A== Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by aserp2130.oracle.com with ESMTP id 2k7a3tcxb2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 20 Jul 2018 02:48:16 +0000 Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w6K2mFx7030249 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 20 Jul 2018 02:48:15 GMT Received: from abhmp0019.oracle.com (abhmp0019.oracle.com [141.146.116.25]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id w6K2mFXq001451; Fri, 20 Jul 2018 02:48:15 GMT Received: from [10.39.210.234] (/10.39.210.234) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 19 Jul 2018 19:48:14 -0700 Subject: Re: [PATCH] Make function clone name numbering independent. From: Michael Ploujnikov To: Richard Biener , Bernhard Fischer Cc: GCC Patches References: <6A1E6E88-63F7-43E4-8A53-78C7694D34BE@gmail.com> <069cb2a6-2e37-58fb-2009-35e0300270f1@oracle.com> Openpgp: preference=signencrypt Message-ID: <618e046e-3f28-f008-237d-497bcff2531e@oracle.com> Date: Fri, 20 Jul 2018 02:49:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <069cb2a6-2e37-58fb-2009-35e0300270f1@oracle.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="woBDCQ3yaB8ZEpf411rGWoOr0pWuZAoNs" X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg01138.txt.bz2 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --woBDCQ3yaB8ZEpf411rGWoOr0pWuZAoNs Content-Type: multipart/mixed; boundary="iK88e3RXovaACm0p2QmD80QN11202olMX"; protected-headers="v1" From: Michael Ploujnikov To: Richard Biener , Bernhard Fischer Cc: GCC Patches Message-ID: <618e046e-3f28-f008-237d-497bcff2531e@oracle.com> Subject: Re: [PATCH] Make function clone name numbering independent. References: <6A1E6E88-63F7-43E4-8A53-78C7694D34BE@gmail.com> <069cb2a6-2e37-58fb-2009-35e0300270f1@oracle.com> In-Reply-To: <069cb2a6-2e37-58fb-2009-35e0300270f1@oracle.com> --iK88e3RXovaACm0p2QmD80QN11202olMX Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Content-length: 7860 On 2018-07-17 04:25 PM, Michael Ploujnikov wrote: > On 2018-07-17 06:02 AM, Richard Biener wrote: >> On Tue, Jul 17, 2018 at 8:10 AM Bernhard Reutner-Fischer >> wrote: >>> >>> On 16 July 2018 21:38:36 CEST, Michael Ploujnikov wrote: >>>> Hi, >>>> >>> >>>> + clone_fn_ids =3D hash_map::create_ggc >>>> (1000); >>> >>> Isn't 1000 a bit excessive? What about 64 or thereabouts? >> >> I'm not sure we should throw memory at this "problem" in this way. >> What specific issue >> does this address? >=20 > This goes along with the general theme of preventing changes to one funct= ion affecting codegen of others. What I'm seeing in this case is when a fun= ction bar is modified codegen decides to create more clones of it (eg: duri= ng the constprop pass). These extra clones cause the global counter to incr= ement so the clones of the unchanged function foo are named differently onl= y because of a source change to bar. I was hoping that the testcase would b= e a good illustration, but perhaps not; is there anything else I can do to = make this clearer? >=20 >=20 >> >> Iff then I belive forgoing the automatic counter addidion is the way >> to go and hand >> control of that to the callers (for example the caller in >> lto/lto-partition.c doesn't >> even seem to need that counter. >=20 > How can you tell that privatize_symbol_name_1 doesn't need the counter? I= 'm assuming it has a good reason to call clone_function_name_1 rather than = appending ".lto_priv" itself. >=20 >> You also assume the string you key on persists - luckily the >> lto-partition.c caller >> leaks it but this makes your approach quite fragile in my eye (put the l= ogic >> into clone_function_name instead, where you at least know you are dealing >> with a string from an indentifier which are never collected). >> >> Richard. >> >=20 > Is this what you had in mind?: >=20 > diff --git gcc/cgraphclones.c gcc/cgraphclones.c > index 6e84a31..f000420 100644 > --- gcc/cgraphclones.c > +++ gcc/cgraphclones.c > @@ -512,7 +512,7 @@ cgraph_node::create_clone (tree new_decl, profile_cou= nt prof_count, > return new_node; > } >=20=20 > -static GTY(()) unsigned int clone_fn_id_num; > +static GTY(()) hash_map *clone_fn_ids; >=20=20 > /* Return a new assembler name for a clone with SUFFIX of a decl named > NAME. */ > @@ -521,14 +521,13 @@ tree > clone_function_name_1 (const char *name, const char *suffix) > { > size_t len =3D strlen (name); > - char *tmp_name, *prefix; > + char *prefix; >=20=20 > prefix =3D XALLOCAVEC (char, len + strlen (suffix) + 2); > memcpy (prefix, name, len); > strcpy (prefix + len + 1, suffix); > prefix[len] =3D symbol_table::symbol_suffix_separator (); > - ASM_FORMAT_PRIVATE_NAME (tmp_name, prefix, clone_fn_id_num++); > - return get_identifier (tmp_name); > + return get_identifier (prefix); > } >=20=20 > /* Return a new assembler name for a clone of DECL with SUFFIX. */ > @@ -537,7 +536,17 @@ tree > clone_function_name (tree decl, const char *suffix) > { > tree name =3D DECL_ASSEMBLER_NAME (decl); > - return clone_function_name_1 (IDENTIFIER_POINTER (name), suffix); > + char *decl_name =3D IDENTIFIER_POINTER (name); > + char *numbered_name; > + unsigned int *suffix_counter; > + if (!clone_fn_ids) { > + /* Initialize the per-function counter hash table if this is the fir= st call */ > + clone_fn_ids =3D hash_map::create_ggc (64); > + } > + suffix_counter =3D &clone_fn_ids->get_or_insert(name); > + ASM_FORMAT_PRIVATE_NAME (numbered_name, decl_name, *suffix_counter); > + *suffix_counter =3D *suffix_counter + 1; > + return clone_function_name_1 (numbered_name, suffix); > } >=20=20 > - Michael >=20 >=20 >=20 Ping, and below is the updated version of the full patch with changelogs: gcc: 2018-07-16 Michael Ploujnikov Make function clone name numbering independent. * cgraphclones.c: Replace clone_fn_id_num with clone_fn_ids. (clone_function_name_1): Move suffixing to clone_function_name and change it to use per-function clone_fn_ids. testsuite: 2018-07-16 Michael Ploujnikov Clone id counters should be completely independent from one another. * gcc/testsuite/gcc.dg/independent-cloneids-1.c: New test. --- gcc/cgraphclones.c | 19 ++++++++++---- gcc/testsuite/gcc.dg/independent-cloneids-1.c | 38 +++++++++++++++++++++++= ++++ 2 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/independent-cloneids-1.c diff --git gcc/cgraphclones.c gcc/cgraphclones.c index 6e84a31..e1a77a2 100644 --- gcc/cgraphclones.c +++ gcc/cgraphclones.c @@ -512,7 +512,7 @@ cgraph_node::create_clone (tree new_decl, profile_count= prof_count, return new_node; } =20 -static GTY(()) unsigned int clone_fn_id_num; +static GTY(()) hash_map *clone_fn_ids; =20 /* Return a new assembler name for a clone with SUFFIX of a decl named NAME. */ @@ -521,14 +521,13 @@ tree clone_function_name_1 (const char *name, const char *suffix) { size_t len =3D strlen (name); - char *tmp_name, *prefix; + char *prefix; =20 prefix =3D XALLOCAVEC (char, len + strlen (suffix) + 2); memcpy (prefix, name, len); strcpy (prefix + len + 1, suffix); prefix[len] =3D symbol_table::symbol_suffix_separator (); - ASM_FORMAT_PRIVATE_NAME (tmp_name, prefix, clone_fn_id_num++); - return get_identifier (tmp_name); + return get_identifier (prefix); } =20 /* Return a new assembler name for a clone of DECL with SUFFIX. */ @@ -537,7 +536,17 @@ tree clone_function_name (tree decl, const char *suffix) { tree name =3D DECL_ASSEMBLER_NAME (decl); - return clone_function_name_1 (IDENTIFIER_POINTER (name), suffix); + const char *decl_name =3D IDENTIFIER_POINTER (name); + char *numbered_name; + unsigned int *suffix_counter; + if (!clone_fn_ids) { + /* Initialize the per-function counter hash table if this is the first= call */ + clone_fn_ids =3D hash_map::create_ggc (64); + } + suffix_counter =3D &clone_fn_ids->get_or_insert(decl_name); + ASM_FORMAT_PRIVATE_NAME (numbered_name, decl_name, *suffix_counter); + *suffix_counter =3D *suffix_counter + 1; + return clone_function_name_1 (numbered_name, suffix); } =20 =20 diff --git gcc/testsuite/gcc.dg/independent-cloneids-1.c gcc/testsuite/gcc.= dg/independent-cloneids-1.c new file mode 100644 index 0000000..d723e20 --- /dev/null +++ gcc/testsuite/gcc.dg/independent-cloneids-1.c @@ -0,0 +1,38 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp" } */ + +extern int printf (const char *, ...); + +static int __attribute__ ((noinline)) +foo (int arg) +{ + return 7 * arg; +} + +static int __attribute__ ((noinline)) +bar (int arg) +{ + return arg * arg; +} + +int +baz (int arg) +{ + printf("%d\n", bar (3)); + printf("%d\n", bar (4)); + printf("%d\n", foo (5)); + printf("%d\n", foo (6)); + /* adding or removing the following call should not affect foo + function's clone numbering */ + printf("%d\n", bar (7)); + return foo (8); +} + +/* { dg-final { scan-ipa-dump "Function bar.constprop.0" "cp" } } */ +/* { dg-final { scan-ipa-dump "Function bar.constprop.1" "cp" } } */ +/* { dg-final { scan-ipa-dump "Function bar.constprop.3" "cp" } } */ +/* { dg-final { scan-ipa-dump "Function foo.constprop.0" "cp" } } */ +/* { dg-final { scan-ipa-dump "Function foo.constprop.1" "cp" } } */ +/* { dg-final { scan-ipa-dump "Function foo.constprop.2" "cp" } } */ +/* { dg-final { scan-ipa-dump-not "Function foo.constprop.3" "cp" } } */ +/* { dg-final { scan-ipa-dump-not "Function foo.constprop.4" "cp" } } */ --=20 2.7.4 --iK88e3RXovaACm0p2QmD80QN11202olMX-- --woBDCQ3yaB8ZEpf411rGWoOr0pWuZAoNs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" Content-length: 473 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJbUU1tAAoJEBnHrw6/GfukIQIH/iFP7yO7vqhAXFWQRD3wQbXf rQIivFiO3whQa3lVhlbaGKtdhVQqYl2DZNWw0YtNjHe5JptQ068LDVUl6jwW/oJp wcD7lzNWwujk55M3RlUCzwSXg3L90Zrzhyerx5lWRkdyN3CSjo4IIW8aOH9WVRRu emJBLfemsC//8DKZIvlNquXcRn4Ft43vz72IAJu8QVielvFdvolZGyiLpDZd0G+T QGLo0hfAC9T8cVc+s9im3zXK0y7NTXcrid9y1LiOtGA79tiW4lZvXDgJpa2J01Bf cz3A1flBrHy9cdnav+52PAlXR6FQJoyBvlY+UP7fLDA/+daq2jQtBx5ijTbw65g= =RwRS -----END PGP SIGNATURE----- --woBDCQ3yaB8ZEpf411rGWoOr0pWuZAoNs--