Ping
2015-05-05 11:06 GMT+03:00 Ilya Enkovich <enkovich....@gmail.com>:
> Ping
>
> 2015-04-14 12:14 GMT+03:00 Ilya Enkovich <enkovich....@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.enkov...@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.enkov...@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;
>> +}
