| Message ID | ri6h820p6tp.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Prevent IPA-SRA from creating calls to local comdats (PR 92676) | expand |
Ping. Thanks, Martin On Mon, Dec 16 2019, Martin Jambor wrote: > Hi, > > since r278669 (fix for PR ipa/91956), IPA-SRA makes sure that the clone > it creates is put into the same same_comdat as the original cgraph_node, > so that it can call private comdats (such as the ipa-split bits of a > comdat that is private). > > However, that means that if there is non-comdat caller of a public > comdat that is modified by IPA-SRA, it now finds itself calling a > private comdat, which call graph verifier does not like (and for a > reason, in theory it can disappear and since it is private it would not > be available from other CUs). > > The patch fixes this by performing the fix for PR 91956 only when the > node in question actually calls a local comdat and when it does, also > making sure that no callers come from a different same_comdat (disabling > IPA-SRA if both conditions are true), so that it plays by the rules in > both modes, does not violate the private comdat calling rule and at the > same time does not disable the transformation unnecessarily. > > The patch also fixes up the calls_comdat_local of callers of the > modified node, despite that not triggering any known issues. It has > passed LTO-bootstrap and testing. What do you think? > > Thanks, > > Martin > > > 2019-12-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/92676 > * ipa-sra.c (struct caller_issues): New fields candidate and > call_from_outside_comdat. > (check_for_caller_issues): Check for calls from outsied of > candidate's same_comdat_group. > (check_all_callers_for_issues): Set up issues.candidate, check result > of the new check. > (process_isra_node_results): Set calls_comdat_local of callers if > appropriate. > --- > gcc/ipa-sra.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index 421c0899e11..8612c8fc920 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -2895,10 +2895,14 @@ ipa_sra_ipa_function_checks (cgraph_node *node) > > struct caller_issues > { > + /* The candidate being considered. */ > + cgraph_node *candidate; > /* There is a thunk among callers. */ > bool thunk; > /* Call site with no available information. */ > bool unknown_callsite; > + /* Call from outside the the candidate's comdat group. */ > + bool call_from_outside_comdat; > /* There is a bit-aligned load into one of non-gimple-typed arguments. */ > bool bit_aligned_aggregate_argument; > }; > @@ -2920,6 +2924,13 @@ check_for_caller_issues (struct cgraph_node *node, void *data) > thunks. */ > return true; > } > + if (issues->candidate->calls_comdat_local > + && issues->candidate->same_comdat_group > + && !issues->candidate->in_same_comdat_group_p (cs->caller)) > + { > + issues->call_from_outside_comdat = true; > + return true; > + } > > isra_call_summary *csum = call_sums->get (cs); > if (!csum) > @@ -2942,6 +2953,7 @@ check_all_callers_for_issues (cgraph_node *node) > { > struct caller_issues issues; > memset (&issues, 0, sizeof (issues)); > + issues.candidate = node; > > node->call_for_symbol_and_aliases (check_for_caller_issues, &issues, true); > if (issues.unknown_callsite) > @@ -2960,6 +2972,13 @@ check_all_callers_for_issues (cgraph_node *node) > node->dump_name ()); > return true; > } > + if (issues.call_from_outside_comdat) > + { > + if (dump_file) > + fprintf (dump_file, "Function would become private comdat called " > + "outside of its comdat group.\n"); > + return true; > + } > > if (issues.bit_aligned_aggregate_argument) > { > @@ -3759,8 +3778,12 @@ process_isra_node_results (cgraph_node *node, > = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", > suffix_counter); > suffix_counter++; > - if (node->same_comdat_group) > - new_node->add_to_same_comdat_group (node); > + if (node->calls_comdat_local && node->same_comdat_group) > + { > + new_node->add_to_same_comdat_group (node); > + for (cgraph_edge *cs = new_node->callers; cs; cs = cs->next_caller) > + cs->caller->calls_comdat_local = true; > + } > new_node->calls_comdat_local = node->calls_comdat_local; > > if (dump_file) > -- > 2.24.0
Hello Honza, ping. Thanks, Martin On Mon, Dec 16 2019, Martin Jambor wrote: > Hi, > > since r278669 (fix for PR ipa/91956), IPA-SRA makes sure that the clone > it creates is put into the same same_comdat as the original cgraph_node, > so that it can call private comdats (such as the ipa-split bits of a > comdat that is private). > > However, that means that if there is non-comdat caller of a public > comdat that is modified by IPA-SRA, it now finds itself calling a > private comdat, which call graph verifier does not like (and for a > reason, in theory it can disappear and since it is private it would not > be available from other CUs). > > The patch fixes this by performing the fix for PR 91956 only when the > node in question actually calls a local comdat and when it does, also > making sure that no callers come from a different same_comdat (disabling > IPA-SRA if both conditions are true), so that it plays by the rules in > both modes, does not violate the private comdat calling rule and at the > same time does not disable the transformation unnecessarily. > > The patch also fixes up the calls_comdat_local of callers of the > modified node, despite that not triggering any known issues. It has > passed LTO-bootstrap and testing. What do you think? > > Thanks, > > Martin > > > 2019-12-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/92676 > * ipa-sra.c (struct caller_issues): New fields candidate and > call_from_outside_comdat. > (check_for_caller_issues): Check for calls from outsied of > candidate's same_comdat_group. > (check_all_callers_for_issues): Set up issues.candidate, check result > of the new check. > (process_isra_node_results): Set calls_comdat_local of callers if > appropriate. > --- > gcc/ipa-sra.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index 421c0899e11..8612c8fc920 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -2895,10 +2895,14 @@ ipa_sra_ipa_function_checks (cgraph_node *node) > > struct caller_issues > { > + /* The candidate being considered. */ > + cgraph_node *candidate; > /* There is a thunk among callers. */ > bool thunk; > /* Call site with no available information. */ > bool unknown_callsite; > + /* Call from outside the the candidate's comdat group. */ > + bool call_from_outside_comdat; > /* There is a bit-aligned load into one of non-gimple-typed arguments. */ > bool bit_aligned_aggregate_argument; > }; > @@ -2920,6 +2924,13 @@ check_for_caller_issues (struct cgraph_node *node, void *data) > thunks. */ > return true; > } > + if (issues->candidate->calls_comdat_local > + && issues->candidate->same_comdat_group > + && !issues->candidate->in_same_comdat_group_p (cs->caller)) > + { > + issues->call_from_outside_comdat = true; > + return true; > + } > > isra_call_summary *csum = call_sums->get (cs); > if (!csum) > @@ -2942,6 +2953,7 @@ check_all_callers_for_issues (cgraph_node *node) > { > struct caller_issues issues; > memset (&issues, 0, sizeof (issues)); > + issues.candidate = node; > > node->call_for_symbol_and_aliases (check_for_caller_issues, &issues, true); > if (issues.unknown_callsite) > @@ -2960,6 +2972,13 @@ check_all_callers_for_issues (cgraph_node *node) > node->dump_name ()); > return true; > } > + if (issues.call_from_outside_comdat) > + { > + if (dump_file) > + fprintf (dump_file, "Function would become private comdat called " > + "outside of its comdat group.\n"); > + return true; > + } > > if (issues.bit_aligned_aggregate_argument) > { > @@ -3759,8 +3778,12 @@ process_isra_node_results (cgraph_node *node, > = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", > suffix_counter); > suffix_counter++; > - if (node->same_comdat_group) > - new_node->add_to_same_comdat_group (node); > + if (node->calls_comdat_local && node->same_comdat_group) > + { > + new_node->add_to_same_comdat_group (node); > + for (cgraph_edge *cs = new_node->callers; cs; cs = cs->next_caller) > + cs->caller->calls_comdat_local = true; > + } > new_node->calls_comdat_local = node->calls_comdat_local; > > if (dump_file) > -- > 2.24.0
Hello, ping. Thanks, Martin On Mon, Dec 16 2019, Martin Jambor wrote: > Hi, > > since r278669 (fix for PR ipa/91956), IPA-SRA makes sure that the clone > it creates is put into the same same_comdat as the original cgraph_node, > so that it can call private comdats (such as the ipa-split bits of a > comdat that is private). > > However, that means that if there is non-comdat caller of a public > comdat that is modified by IPA-SRA, it now finds itself calling a > private comdat, which call graph verifier does not like (and for a > reason, in theory it can disappear and since it is private it would not > be available from other CUs). > > The patch fixes this by performing the fix for PR 91956 only when the > node in question actually calls a local comdat and when it does, also > making sure that no callers come from a different same_comdat (disabling > IPA-SRA if both conditions are true), so that it plays by the rules in > both modes, does not violate the private comdat calling rule and at the > same time does not disable the transformation unnecessarily. > > The patch also fixes up the calls_comdat_local of callers of the > modified node, despite that not triggering any known issues. It has > passed LTO-bootstrap and testing. What do you think? > > Thanks, > > Martin > > > 2019-12-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/92676 > * ipa-sra.c (struct caller_issues): New fields candidate and > call_from_outside_comdat. > (check_for_caller_issues): Check for calls from outsied of > candidate's same_comdat_group. > (check_all_callers_for_issues): Set up issues.candidate, check result > of the new check. > (process_isra_node_results): Set calls_comdat_local of callers if > appropriate. > --- > gcc/ipa-sra.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index 421c0899e11..8612c8fc920 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -2895,10 +2895,14 @@ ipa_sra_ipa_function_checks (cgraph_node *node) > > struct caller_issues > { > + /* The candidate being considered. */ > + cgraph_node *candidate; > /* There is a thunk among callers. */ > bool thunk; > /* Call site with no available information. */ > bool unknown_callsite; > + /* Call from outside the the candidate's comdat group. */ > + bool call_from_outside_comdat; > /* There is a bit-aligned load into one of non-gimple-typed arguments. */ > bool bit_aligned_aggregate_argument; > }; > @@ -2920,6 +2924,13 @@ check_for_caller_issues (struct cgraph_node *node, void *data) > thunks. */ > return true; > } > + if (issues->candidate->calls_comdat_local > + && issues->candidate->same_comdat_group > + && !issues->candidate->in_same_comdat_group_p (cs->caller)) > + { > + issues->call_from_outside_comdat = true; > + return true; > + } > > isra_call_summary *csum = call_sums->get (cs); > if (!csum) > @@ -2942,6 +2953,7 @@ check_all_callers_for_issues (cgraph_node *node) > { > struct caller_issues issues; > memset (&issues, 0, sizeof (issues)); > + issues.candidate = node; > > node->call_for_symbol_and_aliases (check_for_caller_issues, &issues, true); > if (issues.unknown_callsite) > @@ -2960,6 +2972,13 @@ check_all_callers_for_issues (cgraph_node *node) > node->dump_name ()); > return true; > } > + if (issues.call_from_outside_comdat) > + { > + if (dump_file) > + fprintf (dump_file, "Function would become private comdat called " > + "outside of its comdat group.\n"); > + return true; > + } > > if (issues.bit_aligned_aggregate_argument) > { > @@ -3759,8 +3778,12 @@ process_isra_node_results (cgraph_node *node, > = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", > suffix_counter); > suffix_counter++; > - if (node->same_comdat_group) > - new_node->add_to_same_comdat_group (node); > + if (node->calls_comdat_local && node->same_comdat_group) > + { > + new_node->add_to_same_comdat_group (node); > + for (cgraph_edge *cs = new_node->callers; cs; cs = cs->next_caller) > + cs->caller->calls_comdat_local = true; > + } > new_node->calls_comdat_local = node->calls_comdat_local; > > if (dump_file) > -- > 2.24.0
> Hi, > > since r278669 (fix for PR ipa/91956), IPA-SRA makes sure that the clone > it creates is put into the same same_comdat as the original cgraph_node, > so that it can call private comdats (such as the ipa-split bits of a > comdat that is private). > > However, that means that if there is non-comdat caller of a public > comdat that is modified by IPA-SRA, it now finds itself calling a > private comdat, which call graph verifier does not like (and for a > reason, in theory it can disappear and since it is private it would not > be available from other CUs). > > The patch fixes this by performing the fix for PR 91956 only when the > node in question actually calls a local comdat and when it does, also > making sure that no callers come from a different same_comdat (disabling > IPA-SRA if both conditions are true), so that it plays by the rules in > both modes, does not violate the private comdat calling rule and at the > same time does not disable the transformation unnecessarily. > > The patch also fixes up the calls_comdat_local of callers of the > modified node, despite that not triggering any known issues. It has > passed LTO-bootstrap and testing. What do you think? > > Thanks, > > Martin > > > 2019-12-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/92676 > * ipa-sra.c (struct caller_issues): New fields candidate and > call_from_outside_comdat. > (check_for_caller_issues): Check for calls from outsied of > candidate's same_comdat_group. > (check_all_callers_for_issues): Set up issues.candidate, check result > of the new check. > (process_isra_node_results): Set calls_comdat_local of callers if > appropriate. OK except for ... > @@ -3759,8 +3778,12 @@ process_isra_node_results (cgraph_node *node, > = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", > suffix_counter); > suffix_counter++; > - if (node->same_comdat_group) > - new_node->add_to_same_comdat_group (node); > + if (node->calls_comdat_local && node->same_comdat_group) > + { > + new_node->add_to_same_comdat_group (node); > + for (cgraph_edge *cs = new_node->callers; cs; cs = cs->next_caller) > + cs->caller->calls_comdat_local = true; You need to walk all aliases here. > + } > new_node->calls_comdat_local = node->calls_comdat_local; > > if (dump_file) > -- > 2.24.0 >
Hi, On Fri, Mar 20 2020, Jan Hubicka wrote: > [...] > > OK except for ... >> @@ -3759,8 +3778,12 @@ process_isra_node_results (cgraph_node *node, >> = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", >> suffix_counter); >> suffix_counter++; >> - if (node->same_comdat_group) >> - new_node->add_to_same_comdat_group (node); >> + if (node->calls_comdat_local && node->same_comdat_group) >> + { >> + new_node->add_to_same_comdat_group (node); >> + for (cgraph_edge *cs = new_node->callers; cs; cs = cs->next_caller) >> + cs->caller->calls_comdat_local = true; > > You need to walk all aliases here. Oh, right. I assume thunks too? (I acknowledge I swapped most of this out since I fixed this bug and now cannot really say if we can have a thunk there in the call chain.) So like this? Passed bootstrap and testing on x86_64-linux, running LTO bootstrap now. Thanks, Martin 2020-04-01 Martin Jambor <mjambor@suse.cz> PR ipa/92676 * ipa-sra.c (struct caller_issues): New fields candidate and call_from_outside_comdat. (check_for_caller_issues): Check for calls from outsied of candidate's same_comdat_group. (check_all_callers_for_issues): Set up issues.candidate, check result of the new check. (mark_callers_calls_cdlcl): New function. (process_isra_node_results): Set calls_comdat_local of callers if appropriate. --- gcc/ChangeLog | 13 +++++++++++++ gcc/ipa-sra.c | 38 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 654356c8dc8..e9183f4921a 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,16 @@ +2020-04-01 Martin Jambor <mjambor@suse.cz> + + PR ipa/92676 + * ipa-sra.c (struct caller_issues): New fields candidate and + call_from_outside_comdat. + (check_for_caller_issues): Check for calls from outsied of + candidate's same_comdat_group. + (check_all_callers_for_issues): Set up issues.candidate, check result + of the new check. + (mark_callers_calls_cdlcl): New function. + (process_isra_node_results): Set calls_comdat_local of callers if + appropriate. + 2020-04-01 Jakub Jelinek <jakub@redhat.com> PR middle-end/94423 diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c index 31de527d111..45ea62522ab 100644 --- a/gcc/ipa-sra.c +++ b/gcc/ipa-sra.c @@ -2897,10 +2897,14 @@ ipa_sra_ipa_function_checks (cgraph_node *node) struct caller_issues { + /* The candidate being considered. */ + cgraph_node *candidate; /* There is a thunk among callers. */ bool thunk; /* Call site with no available information. */ bool unknown_callsite; + /* Call from outside the the candidate's comdat group. */ + bool call_from_outside_comdat; /* There is a bit-aligned load into one of non-gimple-typed arguments. */ bool bit_aligned_aggregate_argument; }; @@ -2922,6 +2926,13 @@ check_for_caller_issues (struct cgraph_node *node, void *data) thunks. */ return true; } + if (issues->candidate->calls_comdat_local + && issues->candidate->same_comdat_group + && !issues->candidate->in_same_comdat_group_p (cs->caller)) + { + issues->call_from_outside_comdat = true; + return true; + } isra_call_summary *csum = call_sums->get (cs); if (!csum) @@ -2944,6 +2955,7 @@ check_all_callers_for_issues (cgraph_node *node) { struct caller_issues issues; memset (&issues, 0, sizeof (issues)); + issues.candidate = node; node->call_for_symbol_and_aliases (check_for_caller_issues, &issues, true); if (issues.unknown_callsite) @@ -2962,6 +2974,13 @@ check_all_callers_for_issues (cgraph_node *node) node->dump_name ()); return true; } + if (issues.call_from_outside_comdat) + { + if (dump_file) + fprintf (dump_file, "Function would become private comdat called " + "outside of its comdat group.\n"); + return true; + } if (issues.bit_aligned_aggregate_argument) { @@ -3679,6 +3698,17 @@ push_param_adjustments_for_index (isra_func_summary *ifs, unsigned base_index, } } +/* Worker for all call_for_symbol_thunks_and_aliases. Set calls_comdat_local + flag of all callers of NODE. */ + +static bool +mark_callers_calls_cdlcl (struct cgraph_node *node, void *) +{ + for (cgraph_edge *cs = node->callers; cs; cs = cs->next_caller) + cs->caller->calls_comdat_local = true; + return false; +} + /* Do final processing of results of IPA propagation regarding NODE, clone it if appropriate. */ @@ -3763,8 +3793,12 @@ process_isra_node_results (cgraph_node *node, = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", suffix_counter); suffix_counter++; - if (node->same_comdat_group) - new_node->add_to_same_comdat_group (node); + if (node->calls_comdat_local && node->same_comdat_group) + { + new_node->add_to_same_comdat_group (node); + new_node->call_for_symbol_thunks_and_aliases (mark_callers_calls_cdlcl, + NULL, true); + } new_node->calls_comdat_local = node->calls_comdat_local; if (dump_file)
diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c index 421c0899e11..8612c8fc920 100644 --- a/gcc/ipa-sra.c +++ b/gcc/ipa-sra.c @@ -2895,10 +2895,14 @@ ipa_sra_ipa_function_checks (cgraph_node *node) struct caller_issues { + /* The candidate being considered. */ + cgraph_node *candidate; /* There is a thunk among callers. */ bool thunk; /* Call site with no available information. */ bool unknown_callsite; + /* Call from outside the the candidate's comdat group. */ + bool call_from_outside_comdat; /* There is a bit-aligned load into one of non-gimple-typed arguments. */ bool bit_aligned_aggregate_argument; }; @@ -2920,6 +2924,13 @@ check_for_caller_issues (struct cgraph_node *node, void *data) thunks. */ return true; } + if (issues->candidate->calls_comdat_local + && issues->candidate->same_comdat_group + && !issues->candidate->in_same_comdat_group_p (cs->caller)) + { + issues->call_from_outside_comdat = true; + return true; + } isra_call_summary *csum = call_sums->get (cs); if (!csum) @@ -2942,6 +2953,7 @@ check_all_callers_for_issues (cgraph_node *node) { struct caller_issues issues; memset (&issues, 0, sizeof (issues)); + issues.candidate = node; node->call_for_symbol_and_aliases (check_for_caller_issues, &issues, true); if (issues.unknown_callsite) @@ -2960,6 +2972,13 @@ check_all_callers_for_issues (cgraph_node *node) node->dump_name ()); return true; } + if (issues.call_from_outside_comdat) + { + if (dump_file) + fprintf (dump_file, "Function would become private comdat called " + "outside of its comdat group.\n"); + return true; + } if (issues.bit_aligned_aggregate_argument) { @@ -3759,8 +3778,12 @@ process_isra_node_results (cgraph_node *node, = node->create_virtual_clone (callers, NULL, new_adjustments, "isra", suffix_counter); suffix_counter++; - if (node->same_comdat_group) - new_node->add_to_same_comdat_group (node); + if (node->calls_comdat_local && node->same_comdat_group) + { + new_node->add_to_same_comdat_group (node); + for (cgraph_edge *cs = new_node->callers; cs; cs = cs->next_caller) + cs->caller->calls_comdat_local = true; + } new_node->calls_comdat_local = node->calls_comdat_local; if (dump_file)