[PATCHv2] PR ipa/63576: Process speculative edges in ICF
Hi all, This patch is an attempt to fix bug PR ipa/63576, corrected according to note made by Jiong Wang: On 27.10.2014 18:41, Jiong Wang wrote: how about using early exit for above code, something like: if (!e-speculative || profile_status_for_fn (DECL_STRUCT_FUNCTION (dst-decl)) == PROFILE_ABSEN)) { e-count = bb-count; e-frequency = (e-speculative ? CGRAPH_FREQ_BASE : compute_call_stmt_bb_frequency (dst-decl, bb)); return; } gcc_assert (e-count = 0); ... ... Ok, that's right idea. Jan Hubicka wrote: THen you need to sum counts (instead of taking ones from BB) and turn them back to frequencies (because it is profile only counts should be non-0) It seems that counts and frequencies are gathered in some special manner, and this patch simply adds counts from speculative edges and from basic blocks. Of course, I don't know whether this way is proper one, so please correct me or redirect to right place where it is documented. Honza, can you explain your comment to the bug? I've changed the patch, bootstrapped and regtested on x86_64-unknown-linux-gnu again. Ok for trunk? -- Best regards, Ilya Palachev --- gcc/ 2014-10-27 Ilya Palachev i.palac...@samsung.com * ipa-utils.c (compute_edge_count_and_frequency): New function (ipa_merge_profiles): handle speculative case --- gcc/ipa-utils.c | 43 +-- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/gcc/ipa-utils.c b/gcc/ipa-utils.c index e4ea84c..177a170 100644 --- a/gcc/ipa-utils.c +++ b/gcc/ipa-utils.c @@ -390,6 +390,37 @@ get_base_var (tree t) return t; } +/* Computes count and frequency for edges. */ + +static void +compute_edge_count_and_frequency (struct cgraph_edge *e, + struct cgraph_node *dst) +{ + basic_block bb = gimple_bb (e-call_stmt); + if (!e-speculative + || profile_status_for_fn (DECL_STRUCT_FUNCTION (dst-decl)) + == PROFILE_ABSENT) +{ + e-count = bb-count; + e-frequency = compute_call_stmt_bb_frequency (dst-decl, bb); + return; +} + gcc_assert (e-count = 0); + e-count += bb-count; + gcc_assert (e-frequency = 0); + + int entry_freq = ENTRY_BLOCK_PTR_FOR_FN + (DECL_STRUCT_FUNCTION (dst-decl))-frequency; + int freq = e-frequency + bb-frequency; + + if (!entry_freq) +entry_freq = 1, freq++; + + freq = freq * CGRAPH_FREQ_BASE / entry_freq; + if (freq CGRAPH_FREQ_MAX) +freq = CGRAPH_FREQ_MAX; + e-frequency = freq; +} /* SRC and DST are going to be merged. Take SRC's profile and merge it into DST so it is not going to be lost. Destroy SRC's body on the way. */ @@ -547,19 +578,11 @@ ipa_merge_profiles (struct cgraph_node *dst, pop_cfun (); for (e = dst-callees; e; e = e-next_callee) { - gcc_assert (!e-speculative); - e-count = gimple_bb (e-call_stmt)-count; - e-frequency = compute_call_stmt_bb_frequency -(dst-decl, - gimple_bb (e-call_stmt)); + compute_edge_count_and_frequency (e, dst); } for (e = dst-indirect_calls; e; e = e-next_callee) { - gcc_assert (!e-speculative); - e-count = gimple_bb (e-call_stmt)-count; - e-frequency = compute_call_stmt_bb_frequency -(dst-decl, - gimple_bb (e-call_stmt)); + compute_edge_count_and_frequency (e, dst); } src-release_body (); inline_update_overall_summary (dst); -- 2.1.1
[PATCH] PR ipa/63576: Process speculative edges in ICF
Hi all, The attached patch is an attempt to fix the bug PR ipa/63576. As it is said in the comment to the bug, Jan Hubicka wrote: THen you need to sum counts (instead of taking ones from BB) and turn them back to frequencies (because it is profile only counts should be non-0) It seems that counts and frequencies are gathered in some special manner, and this patch simply adds counts from speculative edges and from basic blocks. Of course, I don't know whether this way is proper one, so please correct me or redirect to right place where it is documented :) Anyway, the patch was bootstrapped and regtested on x86_64-unknown-linux-gnu. Ok for trunk? gcc/ 2014-10-22 Ilya Palachev i.palac...@samsung.com * ipa-utils.c (compute_edge_count_and_frequency): New function (ipa_merge_profiles): handle speculative case --- gcc/ipa-utils.c | 52 ++-- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/gcc/ipa-utils.c b/gcc/ipa-utils.c index 552071e..335ab05 100644 --- a/gcc/ipa-utils.c +++ b/gcc/ipa-utils.c @@ -380,6 +380,46 @@ get_base_var (tree t) return t; } +/* Computes count and frequency for edges. */ + +static void +compute_edge_count_and_frequency(struct cgraph_edge *e, +struct cgraph_node *dst) +{ + basic_block bb = gimple_bb (e-call_stmt); + if (!e-speculative) +{ + e-count = bb-count; + e-frequency = compute_call_stmt_bb_frequency (dst-decl, bb); +} + else +{ + if (profile_status_for_fn (DECL_STRUCT_FUNCTION (dst-decl)) + == PROFILE_ABSENT) +{ + e-count = bb-count; + e-frequency = CGRAPH_FREQ_BASE; + } + else +{ + gcc_assert (e-count = 0); + e-count += bb-count; + gcc_assert (e-frequency = 0); + + int entry_freq = ENTRY_BLOCK_PTR_FOR_FN + (DECL_STRUCT_FUNCTION (dst-decl))-frequency; + int freq = e-frequency + bb-frequency; + + if (!entry_freq) + entry_freq = 1, freq++; + + freq = freq * CGRAPH_FREQ_BASE / entry_freq; + if (freq CGRAPH_FREQ_MAX) + freq = CGRAPH_FREQ_MAX; + e-frequency = freq; + } +} +} /* SRC and DST are going to be merged. Take SRC's profile and merge it into DST so it is not going to be lost. Destroy SRC's body on the way. */ @@ -537,19 +577,11 @@ ipa_merge_profiles (struct cgraph_node *dst, pop_cfun (); for (e = dst-callees; e; e = e-next_callee) { - gcc_assert (!e-speculative); - e-count = gimple_bb (e-call_stmt)-count; - e-frequency = compute_call_stmt_bb_frequency -(dst-decl, - gimple_bb (e-call_stmt)); + compute_edge_count_and_frequency(e, dst); } for (e = dst-indirect_calls; e; e = e-next_callee) { - gcc_assert (!e-speculative); - e-count = gimple_bb (e-call_stmt)-count; - e-frequency = compute_call_stmt_bb_frequency -(dst-decl, - gimple_bb (e-call_stmt)); + compute_edge_count_and_frequency(e, dst); } src-release_body (); inline_update_overall_summary (dst); -- 2.1.1
Re: [PATCH] PR lto/61048 Define missed builtins on demand
On 15.10.2014 10:59, Jakub Jelinek wrote: On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: Attached patch fixes PR lto/61048 - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 Given that the patch just replaces an ICE with a necessary link failure, I'd say it is done at the wrong place, instead during the LTO option handling you should error out if there are incompatibilities in -fsanitize options (any object compiled with flag_sanitize SANITIZE_USER_ADDRESS, but link done without that, ditto for SANITIZE_KERNEL_ADDRESS, SANITIZE_THREAD. And finally if flag_sanitize (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) is non-zero but during linking it is zero (it doesn't really matter which exact undefined sanitization options are used at what time). As I understand you suggest that the compiler should: - Write compiler options with which the object file was compiled to the ELF format in a special section. - Read these sections during the linking command - Compare collected sets of options with options that were set at linking command. ? Does compiler have any opportunity to save the set of options with which the object file was compiled inside some special ELF section? For LTO they the special section .gnu.lto_.opts is used. But not all compiler options are written to it: $ gcc -fsanitize=address -c test.cpp -o test.o -flto $ objdump --full-contents --section=.gnu.lto_.opts test.o test.o: file format elf64-x86-64 Contents of section .gnu.lto_.opts: 272d6665 78636570 74696f6e 73272027 '-fexceptions' ' 0010 2d666d61 74682d65 72726e6f 2720272d -fmath-errno' '- 0020 66736967 6e65642d 7a65726f 73272027 fsigned-zeros' ' 0030 2d667472 61707069 6e672d6d 61746827 -ftrapping-math' 0040 20272d66 6e6f2d74 72617076 2720272d '-fno-trapv' '- 0050 666e6f2d 73747269 63742d6f 76657266 fno-strict-overf 0060 6c6f7727 20272d6d 74756e65 3d67656e low' '-mtune=gen 0070 65726963 2720272d 6d617263 683d7838 eric' '-march=x8 0080 362d3634 2720272d 666c746f 2700 6-64' '-flto'. As you can see there is no -fsanitize option here. Only options that are handled in function lto_write_options in lto-opts.c are written there. Does gcc has an opportunity to write something like ELF section .gnu.opts to save all compiler options with which the object file was compiled? Or we need to introduce such section? Or anything else? BTW, in your patches please watch formatting, you didn't use space before (. Sorry for that. I'll check it in further patches. Jakub
Re: [PATCH] PR lto/61048 Define missed builtins on demand
On 15.10.2014 12:09, Richard Biener wrote: On Wed, Oct 15, 2014 at 8:59 AM, Jakub Jelinek ja...@redhat.com wrote: On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: Attached patch fixes PR lto/61048 - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 Given that the patch just replaces an ICE with a necessary link failure, I'd say it is done at the wrong place, instead during the LTO option handling you should error out if there are incompatibilities in -fsanitize options (any object compiled with flag_sanitize SANITIZE_USER_ADDRESS, but link done without that, ditto for SANITIZE_KERNEL_ADDRESS, SANITIZE_THREAD. And finally if flag_sanitize (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) is non-zero but during linking it is zero (it doesn't really matter which exact undefined sanitization options are used at what time). Yep. As with other options this looks like it needs conservative merging. Which might be more involved than for other cases - but well... Look at existing examples in gcc/lto-opts.c and gcc/lto-wrapper.c. I have seen that in r180827 you have removed functions lto_read_file_options from gcc/lto-streamer.h and lto_read_all_file_options from gcc/lto/lto.c. Do you mean that something like these functions should be added again? Maybe it will be worthwhile to report the user what options he forget to add to the linkning command? For example, without -flto option the following error is reported: $ g++ -c test.cpp -fsanitize=address -o test_nolto.o $ g++ test_nolto.o -o test_nolto test_nolto.o:test.cpp:function main: error: undefined reference to '__asan_report_load4' test_nolto.o:test.cpp:function __static_initialization_and_destruction_0(int, int): error: undefined reference to '__asan_before_dynamic_init' test_nolto.o:test.cpp:function __static_initialization_and_destruction_0(int, int): error: undefined reference to '__asan_after_dynamic_init' test_nolto.o:test.cpp:function _GLOBAL__sub_D_00099_0_main: error: undefined reference to '__asan_unregister_globals' test_nolto.o:test.cpp:function _GLOBAL__sub_I_00099_1_main: error: undefined reference to '__asan_init_v4' test_nolto.o:test.cpp:function _GLOBAL__sub_I_00099_1_main: error: undefined reference to '__asan_register_globals' collect2: error: ld returned 1 exit status Maybe it will be better to print something like: test_nolto.o:test.cpp: error: compiled with -fsanitize=address, but this option is not specified in linking command. ? Thanks, Ilya Richard. BTW, in your patches please watch formatting, you didn't use space before (. Jakub
[PATCH] PR lto/61048 Write/read option -fsanitize to/from object files
Hi all, The attached patch fixes PR lto/61048. The basic idea is to write option -fsanitize to existing ELF section .gnu.lto_.opts in object files and then read it in lto-wrapper. On 15.10.2014 12:46, Richard Biener wrote: You need to handle them in lto-opts.c and output them to the existing option section. 2 minor changes are added to existing function that write options (in lto-opts.c) and then read them from object files (lto-wrapper.c). The patch was bootstrapped and regtested on x86_64-unknown-linux-gnu. Ok for trunk? Best regards, Ilya Palachev From e3d1e1dc6d64b04fb81d882528614f1fc37cfa5f Mon Sep 17 00:00:00 2001 From: Ilya Palachev i.palac...@samsung.com Date: Wed, 15 Oct 2014 16:21:50 +0400 Subject: [PATCH] Write/read option -fsanitize to/from object files gcc/ 2014-10-15 Ilya Palachev i.palac...@samsung.com * lto-opts.c (lto_write_options): Enable writing of option -fsanitize to object files. * lto-wrapper.c (run_gcc): Enable reading of option -fsanitize from object files. gcc/testsuite/ 2014-10-15 Ilya Palachev i.palac...@samsung.com * g++.dg/lto/pr61048_0.C: New test from bugzilla. --- gcc/lto-opts.c | 10 +- gcc/lto-wrapper.c| 1 + gcc/testsuite/g++.dg/lto/pr61048_0.C | 9 + 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/lto/pr61048_0.C diff --git a/gcc/lto-opts.c b/gcc/lto-opts.c index ceccb6f..abeca1c 100644 --- a/gcc/lto-opts.c +++ b/gcc/lto-opts.c @@ -178,7 +178,15 @@ lto_write_options (void) which includes things like -o and -v or -fhelp for example. We do not need those. Also drop all diagnostic options. */ if (cl_options[option-opt_index].flags (CL_DRIVER|CL_WARNING)) - continue; +switch (option-opt_index) + { + /* Option -fsanitize should not be dropped, since otherwise + sanitizer builtins will not be initialized. See PR61048. */ + case OPT_fsanitize_: +break; + default: +continue; + } for (j = 0; j option-canonical_option_num_elements; ++j) append_to_collect_gcc_options (temporary_obstack, first_p, diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 8033b15..43d31d7 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -514,6 +514,7 @@ run_gcc (unsigned argc, char *argv[]) case OPT_fwrapv: case OPT_ftrapv: case OPT_fstrict_overflow: + case OPT_fsanitize_: case OPT_O: case OPT_Ofast: case OPT_Og: diff --git a/gcc/testsuite/g++.dg/lto/pr61048_0.C b/gcc/testsuite/g++.dg/lto/pr61048_0.C new file mode 100644 index 000..40ae097 --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr61048_0.C @@ -0,0 +1,9 @@ +// { dg-lto-do link } +// { dg-lto-options {{-fsanitize=address -flto}} } + +#include iostream +int main () +{ + int *i = reinterpret_castint* (0xC100); + std::cout *i std::endl; +} -- 2.1.1
[PATCH] PR lto/61048 Define missed builtins on demand
Hi all, Attached patch fixes PR lto/61048 - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 The reason of failure was that the builtin information structure was not initialized properly at the link stage. The failed assertion was caused by missing builtin declaration ( BUILT_IN_ASAN_AFTER_DYNAMIC_INIT), which was requested from this structure. As usual this information should be initialized in function lto_define_builtins, which is called from LTO lang hook function lto_init. But in the given testcase the initialization did not happen, since the declaration is initialized only if the following condition holds: (flag_sanitize (SANITIZE_ADDRESS | SANITIZE_THREAD \ | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)) But if the user compiles (without linking) file in LTO mode with -fsanitize=address option, and then tries to link the executable from *.o file, but does not specify option -fsanitize=address, variable flag_sanitize will be 0 and sanitizer builtins info will not be initialized, and ICE will happen. Commands to reproduce the problem: g++ test.cpp -c -o test.o -fsanitize=address -flto g++ test.o -o test -Wl,-flto # At this stage flag_sanitize is 0, and sanitizer builtins are not defined. The simplest way to fix this seems to add initialization of sanitizer builtins using function initialize_sanitizer_builtins - and this helps to avoid ICE: diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c index bc53632..f5ca849 100644 --- a/gcc/lto/lto.c +++ b/gcc/lto/lto.c @@ -55,6 +55,7 @@ along with GCC; see the file COPYING3. If not see #include ipa-inline.h #include params.h #include ipa-utils.h +#include asan.h /* Number of parallel tasks to run, -1 if we want to use GNU Make jobserver. */ @@ -1856,6 +1857,9 @@ lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, data_in = lto_data_in_create (decl_data, (const char *) data + string_offset, header-string_size, resolutions); + /* Initialize sanitizer builtins if necessary. */ + initialize_sanitizer_builtins(); + /* We do not uniquify the pre-loaded cache entries, those are middle-end internal types that should not be merged. */ But this approach means that asan-specific functions must be called from lto. The suggested patch proposes another approach: add definitions of builtins during the final stage, when they are requested from builtin_info structure. I have tried to do it by adding lto-specific lang-hook, so that to reuse existing code for builtins initialization (currently builtins are initialized in lto_init hook). In the attached patch such hook is added, and it is used in streamer_get_builtin_tree. It seems that the discussed issue can happen not only for flag -fsanitize, but also for all options that cause the definition of builtins, so the proposed patch is independent from sanitizers. The patch was bootstrapped and regtested on x86_64-unknown-linux-gnu. Ok for trunk? Best regards, Ilya Palachev From 926a8b84a52f3120c3f71cd28e0d782c719b7791 Mon Sep 17 00:00:00 2001 From: Ilya Palachev i.palac...@samsung.com Date: Tue, 14 Oct 2014 19:22:32 +0400 Subject: [PATCH] Define missed builtins on demand gcc/ 2014-10-14 Ilya Palachev i.palac...@samsung.com * langhooks.h (define_builtin_on_demand): New function. * langhooks-def.h (LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND): New macro. * lto/lto-lang.c (lto_define_builtin_on_demand): New function. * tree-streamer-in.c (streamer_get_builtin_tree): Use define_builtin_on_demand in case when the declaration of builtin is missing. gcc/testsuite/ 2014-10-14 Ilya Palachev i.palac...@samsung.com * g++.dg/lto/pr61048_0.C: New test from bugzilla. --- gcc/langhooks-def.h | 4 +++- gcc/langhooks.h | 3 +++ gcc/lto/lto-lang.c | 16 gcc/testsuite/g++.dg/lto/pr61048_0.C | 10 ++ gcc/tree-streamer-in.c | 4 5 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/lto/pr61048_0.C diff --git a/gcc/langhooks-def.h b/gcc/langhooks-def.h index e5ae3e3..2ddccbc 100644 --- a/gcc/langhooks-def.h +++ b/gcc/langhooks-def.h @@ -254,11 +254,13 @@ extern void lhd_end_section (void); #define LANG_HOOKS_BEGIN_SECTION lhd_begin_section #define LANG_HOOKS_APPEND_DATA lhd_append_data #define LANG_HOOKS_END_SECTION lhd_end_section +#define LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND 0 #define LANG_HOOKS_LTO { \ LANG_HOOKS_BEGIN_SECTION, \ LANG_HOOKS_APPEND_DATA, \ - LANG_HOOKS_END_SECTION \ + LANG_HOOKS_END_SECTION, \ + LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND \ } /* The whole thing. The structure is defined in langhooks.h. */ diff --git a/gcc/langhooks.h b/gcc/langhooks.h index 32e76f9..a0cbe5f 100644 --- a/gcc/langhooks.h +++ b/gcc/langhooks.h @@ -255,6 +255,9 @@ struct lang_hooks_for_lto /* End the previously begun LTO section
[PATCH] PR lto/59441 Add initialization and release of bitmap obstack
Hi all, Attached patch fixes PR lto/59441. The reason of failure was that the default bitmap obstack was released just before the execution of early local passes. The error was found using valgrind. It reported that there were 153 invalid reads and 173 invalid writes into the field of the default bitmap obstack structure, and all of them were trying to access data that was free'd previously (at the same point of the program). The solution is to add initialization and release of the bitmap obstack before and after the execution of early local passes. After applying this patch valgrind does not report any errors for the same testcase. The patch was bootstrapped and regtested on x86_64-unknown-linux-gnu. Ok for trunk? Best regards, Ilya Palachev From 9bf2878c0a74475283b5424f24e46b31feb13cf7 Mon Sep 17 00:00:00 2001 From: Ilya Palachev i.palac...@samsung.com Date: Tue, 7 Oct 2014 16:09:25 +0400 Subject: [PATCH] Add initialization and release of bitmap obstack gcc/ 2014-10-07 Ilya Palachev i.palac...@samsung.com * cgraphunit.c (process_new_functions): Add initialization and release of bitmap obstack before and after running of passes. gcc/testsuite/ 2014-10-07 Ilya Palachev i.palac...@samsung.com * g++.dg/lto/pr59441_0.C: New test from bugzilla. --- gcc/cgraphunit.c | 6 +- gcc/testsuite/g++.dg/lto/pr59441_0.C | 26 ++ 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/lto/pr59441_0.C diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index d463505..ee42ad1 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -323,7 +323,11 @@ symbol_table::process_new_functions (void) push_cfun (DECL_STRUCT_FUNCTION (fndecl)); if (state == IPA_SSA !gimple_in_ssa_p (DECL_STRUCT_FUNCTION (fndecl))) - g-get_passes ()-execute_early_local_passes (); + { + bitmap_obstack_initialize (NULL); + g-get_passes ()-execute_early_local_passes (); + bitmap_obstack_release (NULL); + } else if (inline_summary_vec != NULL) compute_inline_parameters (node, true); free_dominance_info (CDI_POST_DOMINATORS); diff --git a/gcc/testsuite/g++.dg/lto/pr59441_0.C b/gcc/testsuite/g++.dg/lto/pr59441_0.C new file mode 100644 index 000..3c766e5 --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr59441_0.C @@ -0,0 +1,26 @@ +// { dg-lto-do assemble } +// { dg-lto-options { { -shared -fPIC -flto -O -fvtable-verify=std } } } + +template typename T struct A +{ + T foo (); +}; + +template typename T struct C: virtual public A T +{ + C operator (C (C )); +}; + +template typename T +C T endl (C int c) +{ + c.foo (); +return c; +} + +C int cout; +void +fn () +{ + cout endl; +} -- 2.1.1