[CHKP, PATCH] Fix LTO cgraph merge for instrumented functions
Ilya Enkovich
enkovich.gnu@gmail.com
Tue May 5 08:06:00 GMT 2015
Ping
2015-04-14 12:14 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> 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 <ilya.enkovich@intel.com>
>
> * 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 <ilya.enkovich@intel.com>
>
> * 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 <cgraph_node *> (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 <cgraph_node *> (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 <cgraph_node *> (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;
> +}
More information about the Gcc-patches
mailing list