This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions
- From: Jan Hubicka <hubicka at ucw dot cz>
- To: Ilya Enkovich <enkovich dot gnu at gmail dot com>
- Cc: Jan Hubicka <hubicka at ucw dot cz>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 29 May 2015 07:13:37 +0200
- Subject: Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions
- Authentication-results: sourceware.org; auth=none
- References: <CAMbmDYZG20UY0pw8X0EmOvUDv7D+qFp6kfGR6zz-xQPAHvp4Uw at mail dot gmail dot com> <20150402170115 dot GB21276 at atrey dot karlin dot mff dot cuni dot cz> <CAMbmDYYHAwu-_b-GwqOnGeY7otapduTKFdh5Ui9Ja8hPEDxSUQ at mail dot gmail dot com> <20150403184915 dot GS21276 at atrey dot karlin dot mff dot cuni dot cz> <CAMbmDYYmSMVZrJk709pFtss3_ML+dx8TW3VMZgW-vdnaPY_H5A at mail dot gmail dot com> <20150410011532 dot GA17443 at kam dot mff dot cuni dot cz> <20150414091414 dot GA50171 at msticlxl57 dot ims dot intel dot com> <CAMbmDYbnTrNifDzquvkKF58nOJunwj2L6VreTnTo1p_48k2c=Q at mail dot gmail dot com> <CAMbmDYZ0w4wQTY7gLDRL8D+d6n-Mto9PoagsX45YZDfLZRmh7w at mail dot gmail dot com> <CAMbmDYb9=99hRQrWOPRzfP7TOFjZdZ1sSFmmNE-Dhd9MuhSEug at mail dot gmail dot com>
> Ping
I am really sorry for ignoring this so long - I would like to reorg the code
and replace instrumentaiton thunks by the notion of transparent aliases,
but did not have time to do that yet. Have quite busy time now.
> >>>
> >>> 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)
reachable.contains (cnode) is !in_boundary_p.
> >>> + && 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);
Why do you need the other tests. Can we have instrumentation node that is not definition?
I suppose you can remove if (cnode->instrumented_version) because you assert it anyway.
> >>> - 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)));
> >>> +
I think we ought to have verify_symtab_node checking for this. All the handling of the
name links seems somewhat fragile (I am mostly concerned about getting it right for
LTO before remaning takes place)
OK with the changes above for mainline and for branch if it does not cause problems
for a week.
Honza
> >>> 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;
> >>> +}