From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103505 invoked by alias); 26 May 2015 12:19:28 -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 103494 invoked by uid 89); 26 May 2015 12:19:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,NO_DNS_FOR_FROM autolearn=no version=3.3.2 X-HELO: mail-oi0-f54.google.com Received: from mail-oi0-f54.google.com (HELO mail-oi0-f54.google.com) (209.85.218.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 26 May 2015 12:19:01 +0000 Received: by oifu123 with SMTP id u123so36456367oif.1 for ; Tue, 26 May 2015 05:18:59 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.182.199.34 with SMTP id jh2mr21624671obc.48.1432642739138; Tue, 26 May 2015 05:18:59 -0700 (PDT) Received: by 10.202.210.84 with HTTP; Tue, 26 May 2015 05:18:59 -0700 (PDT) In-Reply-To: References: <20150312102706.GL27860@msticlxl57.ims.intel.com> <20150319083455.GD64546@msticlxl57.ims.intel.com> <20150402170115.GB21276@atrey.karlin.mff.cuni.cz> <20150403184915.GS21276@atrey.karlin.mff.cuni.cz> <20150410011532.GA17443@kam.mff.cuni.cz> <20150414091414.GA50171@msticlxl57.ims.intel.com> Date: Tue, 26 May 2015 13:09:00 -0000 Message-ID: Subject: Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions From: Ilya Enkovich To: Jan Hubicka Cc: gcc-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg02326.txt.bz2 Ping 2015-05-19 12:40 GMT+03:00 Ilya Enkovich : > Ping > > 2015-05-05 11:06 GMT+03:00 Ilya Enkovich : >> Ping >> >> 2015-04-14 12:14 GMT+03:00 Ilya Enkovich : >>> On 10 Apr 03:15, Jan Hubicka wrote: >>>> > >>>> > References are not streamed out for nodes which are referenced in a >>>> > partition but don't belong to it ('continue' condition in output_refs >>>> > loop). >>>> >>>> Yeah, but it already special cases aliases (because we now always preserve IPA_REF_ALIAS link >>>> in the boundary, so I think making IPA_REF_CHKP to be rpeserved should go the same path) >>>> so we can special case the instrumentation thunks too? >>> >>> Any cgraph_node having instrumented clone should have it, not only instrumentation thunks. Surely we can make an exception for IPA_REF_CHKP. >>> >>>> > >>>> > > >>>> > > I suppose we still need to >>>> > > 1) write_symbol and lto-symtab should know that transparent aliases are not real >>>> > > symbols and ignore them. We have predicate symtab_node::real_symbol_p, >>>> > > I suppose it should return true. >>>> > > >>>> > > I think cgraph_merge and varpool_merge in lto-symtab needs to know what to do >>>> > > when merging two symbols with transparent aliases. >>>> > > 2) At some point we need to populate transparent alias links as discussed in the >>>> > > other thread. >>>> > > 3) For safety, I guess we want symbol_table::change_decl_assembler_name to either >>>> > > sanity check there are no trasparent alias links pointing to old assembler >>>> > > names or update it. >>>> > >>>> > Wouldn't search for possible referring via transparent aliases nodes >>>> > be too expensive? >>>> >>>> Changing of decl_assembler_name is not very common and if we set up the links >>>> only after renaming of statics, i think we are safe. But it would be better to >>>> keep verify it at some point. >>>> >>>> I suppose verify_node check that there is no transparent alias link on symbols >>>> were it is not supposed to be and that there is link when it is supposed to be (i.e. >>>> symtab_state is >=EXPANSION and symbol is either weakref on tragets w/o native weakrefs >>>> or instrumentation cone. >>>> >>>> Would you please mind adding the test and making sure it triggers when things are >>>> out of sync? >>>> >>> >>> OK, but I suppose it should be a part of transparent alias links rework discussed in another thread. BTW are you going to intoroduce transparent aliases in cgraph soon? >>> >>>> > >>>> > > 4) I think we want verify_node to check that transparent alias links and chkp points >>>> > > as intended and only on those symbols where they need to be >>>> > > >>>> > > There is also logic in lto-partition to always insert alias target >>>> > >> > OK, so you have the chkp references that links to instrumented_version. >>>> > >> > You do not stream them for boundary symbols but for some reason they are needed, >>>> > >> > but when you can end up with wrong CHKP link? >>>> > >> >>>> > >> Wrong links may appear when cgraph nodes are merged. >>>> > > >>>> > > I see, I think you really want to fix these at merging time as sugested in 1). >>>> > > In this case even the node->instrumented_version pointer may be out of date and >>>> > > point to a node that lost in merging? >>>> > >>>> > node->instrumented_version is redirected and never points to lost >>>> > symbol. But during merge we may have situations when >>>> > (node->instrumented_version->instrumented_version != node) because of >>>> > multiple not merged (yet) symbols referencing the same merged target. >>>> >>>> OK, which should not be a problem if we stream the links instead of rebuilding them, right? >>> >>> IPA_REF_CHKP refs don't affect instrumented_version merging. And I actually don't see here any problems, instrumented_version links always come into consistent state when symbol merging finishes. >>> >>> >>> Here is a new patch version. It has no chkp_maybe_fix_chkp_ref function, IPA_REF_CHKP references are streamed out instead. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? I also want to port it to GCC 5 branch after release. >>> >>> Thanks, >>> Ilya >>> -- >>> gcc/ >>> >>> 2015-04-14 Ilya Enkovich >>> >>> * ipa.c (symbol_table::remove_unreachable_nodes): Don't >>> remove instumentation thunks calling reachable functions. >>> * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP. >>> * lto/lto-partition.c (privatize_symbol_name_1): New. >>> (privatize_symbol_name): Privatize both decl and orig_decl >>> names for instrumented functions. >>> >>> gcc/testsuite/ >>> >>> 2015-04-14 Ilya Enkovich >>> >>> * gcc.dg/lto/chkp-privatize-1_0.c: New. >>> * gcc.dg/lto/chkp-privatize-1_1.c: New. >>> * gcc.dg/lto/chkp-privatize-2_0.c: New. >>> * gcc.dg/lto/chkp-privatize-2_1.c: New. >>> >>> >>> diff --git a/gcc/ipa.c b/gcc/ipa.c >>> index b3752de..3054afe 100644 >>> --- a/gcc/ipa.c >>> +++ b/gcc/ipa.c >>> @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) >>> } >>> else if (cnode->thunk.thunk_p) >>> enqueue_node (cnode->callees->callee, &first, &reachable); >>> - >>> + >>> + /* For instrumentation clones we always need original >>> + function node for proper LTO privatization. */ >>> + if (cnode->instrumentation_clone >>> + && reachable.contains (cnode) >>> + && cnode->definition) >>> + { >>> + gcc_assert (cnode->instrumented_version || in_lto_p); >>> + if (cnode->instrumented_version) >>> + { >>> + enqueue_node (cnode->instrumented_version, &first, >>> + &reachable); >>> + reachable.add (cnode->instrumented_version); >>> + } >>> + } >>> + >>> /* If any reachable function has simd clones, mark them as >>> reachable as well. */ >>> if (cnode->simd_clones) >>> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c >>> index ac50e4b..ea352f1 100644 >>> --- a/gcc/lto-cgraph.c >>> +++ b/gcc/lto-cgraph.c >>> @@ -805,8 +805,33 @@ output_refs (lto_symtab_encoder_t encoder) >>> { >>> symtab_node *node = lto_symtab_encoder_deref (encoder, i); >>> >>> + /* IPA_REF_ALIAS and IPA_REF_CHKP references are always preserved >>> + in the boundary. Alias node can't have other references and >>> + can be always handled as if it's not in the boundary. */ >>> if (!node->alias && !lto_symtab_encoder_in_partition_p (encoder, node)) >>> - continue; >>> + { >>> + cgraph_node *cnode = dyn_cast (node); >>> + /* Output IPA_REF_CHKP reference. */ >>> + if (cnode >>> + && cnode->instrumented_version >>> + && !cnode->instrumentation_clone) >>> + { >>> + for (int i = 0; node->iterate_reference (i, ref); i++) >>> + if (ref->use == IPA_REF_CHKP) >>> + { >>> + if (lto_symtab_encoder_lookup (encoder, ref->referred) >>> + != LCC_NOT_FOUND) >>> + { >>> + int nref = lto_symtab_encoder_lookup (encoder, node); >>> + streamer_write_gcov_count_stream (ob->main_stream, 1); >>> + streamer_write_uhwi_stream (ob->main_stream, nref); >>> + lto_output_ref (ob, ref, encoder); >>> + } >>> + break; >>> + } >>> + } >>> + continue; >>> + } >>> >>> count = node->ref_list.nreferences (); >>> if (count) >>> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c >>> index 235b735..7d117e9 100644 >>> --- a/gcc/lto/lto-partition.c >>> +++ b/gcc/lto/lto-partition.c >>> @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node) >>> } >>> } >>> >>> -/* Mangle NODE symbol name into a local name. >>> - This is necessary to do >>> - 1) if two or more static vars of same assembler name >>> - are merged into single ltrans unit. >>> - 2) if previously static var was promoted hidden to avoid possible conflict >>> - with symbols defined out of the LTO world. */ >>> +/* Helper for privatize_symbol_name. Mangle NODE symbol name >>> + represented by DECL. */ >>> >>> static bool >>> -privatize_symbol_name (symtab_node *node) >>> +privatize_symbol_name_1 (symtab_node *node, tree decl) >>> { >>> - tree decl = node->decl; >>> - const char *name; >>> - cgraph_node *cnode = dyn_cast (node); >>> - >>> - /* If we want to privatize instrumentation clone >>> - then we need to change original function name >>> - which is used via transparent alias chain. */ >>> - if (cnode && cnode->instrumentation_clone) >>> - decl = cnode->orig_decl; >>> - >>> - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); >>> + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); >>> >>> if (must_not_rename (node, name)) >>> return false; >>> @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node) >>> symtab->change_decl_assembler_name (decl, >>> clone_function_name_1 (name, >>> "lto_priv")); >>> + >>> if (node->lto_file_data) >>> lto_record_renamed_decl (node->lto_file_data, name, >>> IDENTIFIER_POINTER >>> (DECL_ASSEMBLER_NAME (decl))); >>> + >>> + if (symtab->dump_file) >>> + fprintf (symtab->dump_file, >>> + "Privatizing symbol name: %s -> %s\n", >>> + name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); >>> + >>> + return true; >>> +} >>> + >>> +/* Mangle NODE symbol name into a local name. >>> + This is necessary to do >>> + 1) if two or more static vars of same assembler name >>> + are merged into single ltrans unit. >>> + 2) if previously static var was promoted hidden to avoid possible conflict >>> + with symbols defined out of the LTO world. */ >>> + >>> +static bool >>> +privatize_symbol_name (symtab_node *node) >>> +{ >>> + if (!privatize_symbol_name_1 (node, node->decl)) >>> + return false; >>> + >>> /* We could change name which is a target of transparent alias >>> chain of instrumented function name. Fix alias chain if so .*/ >>> - if (cnode) >>> + if (cgraph_node *cnode = dyn_cast (node)) >>> { >>> tree iname = NULL_TREE; >>> if (cnode->instrumentation_clone) >>> - iname = DECL_ASSEMBLER_NAME (cnode->decl); >>> + { >>> + /* If we want to privatize instrumentation clone >>> + then we also need to privatize original function. */ >>> + if (cnode->instrumented_version) >>> + privatize_symbol_name (cnode->instrumented_version); >>> + else >>> + privatize_symbol_name_1 (cnode, cnode->orig_decl); >>> + iname = DECL_ASSEMBLER_NAME (cnode->decl); >>> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); >>> + } >>> else if (cnode->instrumented_version >>> - && cnode->instrumented_version->orig_decl == decl) >>> - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); >>> - >>> - if (iname) >>> + && cnode->instrumented_version->orig_decl == cnode->decl) >>> { >>> - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); >>> - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); >>> + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); >>> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); >>> } >>> } >>> - if (symtab->dump_file) >>> - fprintf (symtab->dump_file, >>> - "Privatizing symbol name: %s -> %s\n", >>> - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); >>> + >>> return true; >>> } >>> >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c >>> new file mode 100644 >>> index 0000000..2054aa15 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c >>> @@ -0,0 +1,17 @@ >>> +/* { dg-lto-do link } */ >>> +/* { dg-require-effective-target mpx } */ >>> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ >>> + >>> +extern int __attribute__((noinline)) f1 (int i); >>> + >>> +static int __attribute__((noinline)) >>> +f2 (int i) >>> +{ >>> + return i + 6; >>> +} >>> + >>> +int >>> +main (int argc, char **argv) >>> +{ >>> + return f1 (argc) + f2 (argc); >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c >>> new file mode 100644 >>> index 0000000..4fa8656 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c >>> @@ -0,0 +1,11 @@ >>> +static int __attribute__((noinline)) >>> +f2 (int i) >>> +{ >>> + return 2 * i; >>> +} >>> + >>> +int __attribute__((noinline)) >>> +f1 (int i) >>> +{ >>> + return f2 (i) + 10; >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c >>> new file mode 100644 >>> index 0000000..be7f601 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c >>> @@ -0,0 +1,18 @@ >>> +/* { dg-lto-do link } */ >>> +/* { dg-require-effective-target mpx } */ >>> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ >>> + >>> +static int >>> +__attribute__ ((noinline)) >>> +func1 (int i) >>> +{ >>> + return i + 2; >>> +} >>> + >>> +extern int func2 (int i); >>> + >>> +int >>> +main (int argc, char **argv) >>> +{ >>> + return func1 (argc) + func2 (argc) + 1; >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c >>> new file mode 100644 >>> index 0000000..db39e7f >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c >>> @@ -0,0 +1,8 @@ >>> +int func1 = 10; >>> + >>> +int >>> +func2 (int i) >>> +{ >>> + func1++; >>> + return i + func1; >>> +}