Re: [PATCH 2/3] ipa: Prune any IPA-CP aggregate constants known by modref to be killed (111157)
Hello,
On Thu, Oct 05 2023, Jan Hubicka wrote:
>> gcc/ChangeLog:
>>
>> 2023-09-19 Martin Jambor
>>
>> PR ipa/57
>> * ipa-prop.h (struct ipa_argagg_value): Newf flag killed.
>> * ipa-modref.cc (ipcp_argagg_and_kill_overlap_p): New function.
>> (update_signature): Mark any any IPA-CP aggregate constants at
>> positions known to be killed as killed. Move check that there is
>> clone_info after this pruning.
>> * ipa-cp.cc (ipa_argagg_value_list::dump): Dump the killed flag.
>> (ipa_argagg_value_list::push_adjusted_values): Clear the new flag.
>> (push_agg_values_from_plats): Likewise.
>> (ipa_push_agg_values_from_jfunc): Likewise.
>> (estimate_local_effects): Likewise.
>> (push_agg_values_for_index_from_edge): Likewise.
>> * ipa-prop.cc (write_ipcp_transformation_info): Stream the killed
>> flag.
>> (read_ipcp_transformation_info): Likewise.
>> (ipcp_get_aggregate_const): Update comment, assert that encountered
>> record does not have killed flag set.
>> (ipcp_transform_function): Prune all aggregate constants with killed
>> set.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2023-09-18 Martin Jambor
>>
>> PR ipa/57
>> * gcc.dg/lto/pr57_0.c: New test.
>> * gcc.dg/lto/pr57_1.c: Second file of the same new test.
>
>> diff --git a/gcc/ipa-modref.cc b/gcc/ipa-modref.cc
>> index c04f9f44c06..a8fcf159259 100644
>> --- a/gcc/ipa-modref.cc
>> +++ b/gcc/ipa-modref.cc
>> @@ -4065,21 +4065,71 @@ remap_kills (vec , const
>> vec )
>>i++;
>> }
>>
>> +/* Return true if the V can overlap with KILL. */
>> +
>> +static bool
>> +ipcp_argagg_and_kill_overlap_p (const ipa_argagg_value ,
>> +const modref_access_node )
>> +{
>> + if (kill.parm_index == v.index)
>> +{
>> + gcc_assert (kill.parm_offset_known);
>> + gcc_assert (known_eq (kill.max_size, kill.size));
>> + poly_int64 repl_size;
>> + bool ok = poly_int_tree_p (TYPE_SIZE (TREE_TYPE (v.value)),
>> + _size);
>> + gcc_assert (ok);
>> + poly_int64 repl_offset (v.unit_offset);
>> + repl_offset <<= LOG2_BITS_PER_UNIT;
>> + poly_int64 combined_offset
>> += (kill.parm_offset << LOG2_BITS_PER_UNIT) + kill.offset;
> parm_offset may be negative which I think will confuse
> ranges_maybe_overlap_p.
> I think you need to test for this and if it is negative adjust
> repl_offset instead of kill.offset
After a discussion with Honza about this in person, we came to the
conclusion that the patch works as intended even in presence of negative
parm_offsets (I even have a testcase but I need to enhance IPA-CP a bit
in order for it to be useful also outside a debugger).
>> + if (ranges_maybe_overlap_p (repl_offset, repl_size,
>> + combined_offset, kill.size))
>> +return true;
>> +}
>> + return false;
>> +}
>> +
>> /* If signature changed, update the summary. */
>>
>> static void
>> update_signature (struct cgraph_node *node)
>> {
>> - clone_info *info = clone_info::get (node);
>> - if (!info || !info->param_adjustments)
>> -return;
>> -
>>modref_summary *r = optimization_summaries
>>? optimization_summaries->get (node) : NULL;
>>modref_summary_lto *r_lto = summaries_lto
>>? summaries_lto->get (node) : NULL;
>>if (!r && !r_lto)
>> return;
>> +
>> + ipcp_transformation *ipcp_ts = ipcp_get_transformation_summary (node);
> Please add comment on why this is necessary.
>> + if (ipcp_ts)
>> +{
>> +for (auto : ipcp_ts->m_agg_values)
>> + {
>> +if (!v.by_ref)
>> + continue;
>> +if (r)
>> + for (const modref_access_node : r->kills)
>> +if (ipcp_argagg_and_kill_overlap_p (v, kill))
>> + {
>> +v.killed = true;
>> +break;
>> + }
>> +if (!v.killed && r_lto)
>> + for (const modref_access_node : r_lto->kills)
>> +if (ipcp_argagg_and_kill_overlap_p (v, kill))
>> + {
>> +v.killed = 1;
> = true?
>> +break;
>> + }
>> + }
>> +}
>> +
>> + clone_info *info = clone_info::get (node);
>> + if (!info || !info->param_adjustments)
>> +return;
>> +
> OK.
This is what I am about to commit.
Thanks,
Martin
PR 57 shows that IPA-modref and IPA-CP (when plugged into value
numbering) can optimize out a store both before a call (because the
call will overwrite it) and in the call (because the store is of the
same value) and by eliminating both create miscompilation.
This patch fixes that by pruning any constants from the list of IPA-CP
aggregate value constants that it knows the contents of the memory can
be "killed." Unfortunately, doing so is tricky. First, IPA-modref
loads override kills and so only stores not loaded are truly not
necessary. Looking stuff up there means doing what most of
Re: [PATCH 2/3] ipa: Prune any IPA-CP aggregate constants known by modref to be killed (111157)
> gcc/ChangeLog:
>
> 2023-09-19 Martin Jambor
>
> PR ipa/57
> * ipa-prop.h (struct ipa_argagg_value): Newf flag killed.
> * ipa-modref.cc (ipcp_argagg_and_kill_overlap_p): New function.
> (update_signature): Mark any any IPA-CP aggregate constants at
> positions known to be killed as killed. Move check that there is
> clone_info after this pruning.
> * ipa-cp.cc (ipa_argagg_value_list::dump): Dump the killed flag.
> (ipa_argagg_value_list::push_adjusted_values): Clear the new flag.
> (push_agg_values_from_plats): Likewise.
> (ipa_push_agg_values_from_jfunc): Likewise.
> (estimate_local_effects): Likewise.
> (push_agg_values_for_index_from_edge): Likewise.
> * ipa-prop.cc (write_ipcp_transformation_info): Stream the killed
> flag.
> (read_ipcp_transformation_info): Likewise.
> (ipcp_get_aggregate_const): Update comment, assert that encountered
> record does not have killed flag set.
> (ipcp_transform_function): Prune all aggregate constants with killed
> set.
>
> gcc/testsuite/ChangeLog:
>
> 2023-09-18 Martin Jambor
>
> PR ipa/57
> * gcc.dg/lto/pr57_0.c: New test.
> * gcc.dg/lto/pr57_1.c: Second file of the same new test.
> diff --git a/gcc/ipa-modref.cc b/gcc/ipa-modref.cc
> index c04f9f44c06..a8fcf159259 100644
> --- a/gcc/ipa-modref.cc
> +++ b/gcc/ipa-modref.cc
> @@ -4065,21 +4065,71 @@ remap_kills (vec , const
> vec )
>i++;
> }
>
> +/* Return true if the V can overlap with KILL. */
> +
> +static bool
> +ipcp_argagg_and_kill_overlap_p (const ipa_argagg_value ,
> + const modref_access_node )
> +{
> + if (kill.parm_index == v.index)
> +{
> + gcc_assert (kill.parm_offset_known);
> + gcc_assert (known_eq (kill.max_size, kill.size));
> + poly_int64 repl_size;
> + bool ok = poly_int_tree_p (TYPE_SIZE (TREE_TYPE (v.value)),
> + _size);
> + gcc_assert (ok);
> + poly_int64 repl_offset (v.unit_offset);
> + repl_offset <<= LOG2_BITS_PER_UNIT;
> + poly_int64 combined_offset
> + = (kill.parm_offset << LOG2_BITS_PER_UNIT) + kill.offset;
parm_offset may be negative which I think will confuse ranges_maybe_overlap_p.
I think you need to test for this and if it is negative adjust
repl_offset instead of kill.offset
> + if (ranges_maybe_overlap_p (repl_offset, repl_size,
> + combined_offset, kill.size))
> + return true;
> +}
> + return false;
> +}
> +
> /* If signature changed, update the summary. */
>
> static void
> update_signature (struct cgraph_node *node)
> {
> - clone_info *info = clone_info::get (node);
> - if (!info || !info->param_adjustments)
> -return;
> -
>modref_summary *r = optimization_summaries
> ? optimization_summaries->get (node) : NULL;
>modref_summary_lto *r_lto = summaries_lto
> ? summaries_lto->get (node) : NULL;
>if (!r && !r_lto)
> return;
> +
> + ipcp_transformation *ipcp_ts = ipcp_get_transformation_summary (node);
Please add comment on why this is necessary.
> + if (ipcp_ts)
> +{
> +for (auto : ipcp_ts->m_agg_values)
> + {
> + if (!v.by_ref)
> + continue;
> + if (r)
> + for (const modref_access_node : r->kills)
> + if (ipcp_argagg_and_kill_overlap_p (v, kill))
> + {
> + v.killed = true;
> + break;
> + }
> + if (!v.killed && r_lto)
> + for (const modref_access_node : r_lto->kills)
> + if (ipcp_argagg_and_kill_overlap_p (v, kill))
> + {
> + v.killed = 1;
= true?
> + break;
> + }
> + }
> +}
> +
> + clone_info *info = clone_info::get (node);
> + if (!info || !info->param_adjustments)
> +return;
> +
OK.
Honza
[PATCH 2/3] ipa: Prune any IPA-CP aggregate constants known by modref to be killed (111157)
PR 57 shows that IPA-modref and IPA-CP (when plugged into value numbering) can optimize out a store both before a call (because the call will overwrite it) and in the call (because the store is of the same value) and by eliminating both create miscompilation. This patch fixes that by pruning any constants from the list of IPA-CP aggregate value constants that it knows the contents of the memory can be "killed." Unfortunately, doing so is tricky. First, IPA-modref loads override kills and so only stores not loaded are truly not necessary. Looking stuff up there means doing what most of what modref_may_alias may do but doing exactly what it does is tricky because it takes also aliasing into account and has bail-out counters. To err on the side of caution in order to avoid this miscompilation we have to prune a constant when in doubt. However, pruning can interfere with the mechanism of how clone materialization distinguishes between the cases when a parameter was entirely removed and when it was both IPA-CPed and IPA-SRAed (in order to make up for the removal in debug info, which can bump into an assert when compiling g++.dg/torture/pr103669.C when we are not careful). Therefore this patch: 1) marks constants that IPA-modref has in its kill list with a new "killed" flag, and 2) prunes the list from entries with this flag after materialization and IPA-CP transformation is done using the template introduced in the previous patch It does not try to look up anything in the load lists, this will be done as a follow-up in order to ease review. gcc/ChangeLog: 2023-09-19 Martin Jambor PR ipa/57 * ipa-prop.h (struct ipa_argagg_value): Newf flag killed. * ipa-modref.cc (ipcp_argagg_and_kill_overlap_p): New function. (update_signature): Mark any any IPA-CP aggregate constants at positions known to be killed as killed. Move check that there is clone_info after this pruning. * ipa-cp.cc (ipa_argagg_value_list::dump): Dump the killed flag. (ipa_argagg_value_list::push_adjusted_values): Clear the new flag. (push_agg_values_from_plats): Likewise. (ipa_push_agg_values_from_jfunc): Likewise. (estimate_local_effects): Likewise. (push_agg_values_for_index_from_edge): Likewise. * ipa-prop.cc (write_ipcp_transformation_info): Stream the killed flag. (read_ipcp_transformation_info): Likewise. (ipcp_get_aggregate_const): Update comment, assert that encountered record does not have killed flag set. (ipcp_transform_function): Prune all aggregate constants with killed set. gcc/testsuite/ChangeLog: 2023-09-18 Martin Jambor PR ipa/57 * gcc.dg/lto/pr57_0.c: New test. * gcc.dg/lto/pr57_1.c: Second file of the same new test. --- gcc/ipa-cp.cc | 8 gcc/ipa-modref.cc | 58 +-- gcc/ipa-prop.cc | 17 +++- gcc/ipa-prop.h| 4 ++ gcc/testsuite/gcc.dg/lto/pr57_0.c | 24 +++ gcc/testsuite/gcc.dg/lto/pr57_1.c | 10 + 6 files changed, 115 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/lto/pr57_0.c create mode 100644 gcc/testsuite/gcc.dg/lto/pr57_1.c diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc index 071c607fbe8..bb49a1b2959 100644 --- a/gcc/ipa-cp.cc +++ b/gcc/ipa-cp.cc @@ -1271,6 +1271,8 @@ ipa_argagg_value_list::dump (FILE *f) print_generic_expr (f, av.value); if (av.by_ref) fprintf (f, "(by_ref)"); + if (av.killed) + fprintf (f, "(killed)"); comma = true; } fprintf (f, "\n"); @@ -1437,6 +1439,8 @@ ipa_argagg_value_list::push_adjusted_values (unsigned src_index, new_av.unit_offset = av->unit_offset - unit_delta; new_av.index = dest_index; new_av.by_ref = av->by_ref; + gcc_assert (!av->killed); + new_av.killed = false; /* Quick check that the offsets we push are indeed increasing. */ gcc_assert (first @@ -1473,6 +1477,7 @@ push_agg_values_from_plats (ipcp_param_lattices *plats, int dest_index, iav.unit_offset = aglat->offset / BITS_PER_UNIT - unit_delta; iav.index = dest_index; iav.by_ref = plats->aggs_by_ref; + iav.killed = false; gcc_assert (first || iav.unit_offset > prev_unit_offset); @@ -2139,6 +2144,7 @@ ipa_push_agg_values_from_jfunc (ipa_node_params *info, cgraph_node *node, iav.unit_offset = item.offset / BITS_PER_UNIT; iav.index = dst_index; iav.by_ref = agg_jfunc->by_ref; + iav.killed = 0; gcc_assert (first || iav.unit_offset > prev_unit_offset); @@ -3970,6 +3976,7 @@ estimate_local_effects (struct cgraph_node *node) avals.m_known_aggs[j].unit_offset = unit_offset;
