Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

[Ilya Enkovich <enkovich.gnu@gmail.com>]

Ping

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