Ping.
On Mon, Jun 21 2021, Martin Jambor wrote:
> Hi,
>
> The "new" IPA-SRA has a more difficult job than the previous
> not-truly-IPA version when identifying situations in which a parameter
> passed by reference can be passed into a third function and only thee
> converted to one passed by value (and possibly "split" at the same
> time).
>
> In order to allow this, two conditions must be fulfilled. First the
> call to the third function must happen before any modifications of
> memory, because it could change the value passed by reference.
> Second, in order to make sure we do not introduce new (invalid)
> dereferences, the call must postdominate the entry BB.
>
> The second condition is actually not necessary if the caller function
> is also certain to dereference the pointer but the first one must
> still hold. Unfortunately, the code making this overriding decision
> also happen to trigger when the first condition is not fulfilled.
> This is fixed in the following patch.
>
> Bootstrapped, LTO-bootstrapped and tested on x86_64-linux, OK for trunk
> and the gcc-11 branch? On gcc-10, I might just remove the override
> altogether, the case might not be important enough to change LTO format.
>
> Thanks,
>
> Martin
>
>
>
> gcc/ChangeLog:
>
> 2021-06-16 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/101066
> * ipa-sra.c (class isra_call_summary): New member
> m_before_any_store, initialize it in the constructor.
> (isra_call_summary::dump): Dump the new field.
> (ipa_sra_call_summaries::duplicate): Copy it.
> (process_scan_results): Set it.
> (isra_write_edge_summary): Stream it.
> (isra_read_edge_summary): Likewise.
> (param_splitting_across_edge): Only override
> safe_to_import_accesses if m_before_any_store is set.
>
> gcc/testsuite/ChangeLog:
>
> 2021-06-16 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/101066
> * gcc.dg/ipa/pr101066.c: New test.
> ---
> gcc/ipa-sra.c | 15 +++++++++++++--
> gcc/testsuite/gcc.dg/ipa/pr101066.c | 20 ++++++++++++++++++++
> 2 files changed, 33 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr101066.c
>
> diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
> index 3272daf56e4..965e246d788 100644
> --- a/gcc/ipa-sra.c
> +++ b/gcc/ipa-sra.c
> @@ -343,7 +343,7 @@ class isra_call_summary
> public:
> isra_call_summary ()
> : m_arg_flow (), m_return_ignored (false), m_return_returned (false),
> - m_bit_aligned_arg (false)
> + m_bit_aligned_arg (false), m_before_any_store (false)
> {}
>
> void init_inputs (unsigned arg_count);
> @@ -362,6 +362,10 @@ public:
>
> /* Set when any of the call arguments are not byte-aligned. */
> unsigned m_bit_aligned_arg : 1;
> +
> + /* Set to true if the call happend before any (other) store to memory in
> the
> + caller. */
> + unsigned m_before_any_store : 1;
> };
>
> /* Class to manage function summaries. */
> @@ -491,6 +495,8 @@ isra_call_summary::dump (FILE *f)
> fprintf (f, " return value ignored\n");
> if (m_return_returned)
> fprintf (f, " return value used only to compute caller return
> value\n");
> + if (m_before_any_store)
> + fprintf (f, " happens before any store to memory\n");
> for (unsigned i = 0; i < m_arg_flow.length (); i++)
> {
> fprintf (f, " Parameter %u:\n", i);
> @@ -535,6 +541,7 @@ ipa_sra_call_summaries::duplicate (cgraph_edge *,
> cgraph_edge *,
> new_sum->m_return_ignored = old_sum->m_return_ignored;
> new_sum->m_return_returned = old_sum->m_return_returned;
> new_sum->m_bit_aligned_arg = old_sum->m_bit_aligned_arg;
> + new_sum->m_before_any_store = old_sum->m_before_any_store;
> }
>
>
> @@ -2374,6 +2381,7 @@ process_scan_results (cgraph_node *node, struct
> function *fun,
> unsigned count = gimple_call_num_args (call_stmt);
> isra_call_summary *csum = call_sums->get_create (cs);
> csum->init_inputs (count);
> + csum->m_before_any_store = uses_memory_as_obtained;
> for (unsigned argidx = 0; argidx < count; argidx++)
> {
> if (!csum->m_arg_flow[argidx].pointer_pass_through)
> @@ -2546,6 +2554,7 @@ isra_write_edge_summary (output_block *ob, cgraph_edge
> *e)
> bp_pack_value (&bp, csum->m_return_ignored, 1);
> bp_pack_value (&bp, csum->m_return_returned, 1);
> bp_pack_value (&bp, csum->m_bit_aligned_arg, 1);
> + bp_pack_value (&bp, csum->m_before_any_store, 1);
> streamer_write_bitpack (&bp);
> }
>
> @@ -2664,6 +2673,7 @@ isra_read_edge_summary (struct lto_input_block *ib,
> cgraph_edge *cs)
> csum->m_return_ignored = bp_unpack_value (&bp, 1);
> csum->m_return_returned = bp_unpack_value (&bp, 1);
> csum->m_bit_aligned_arg = bp_unpack_value (&bp, 1);
> + csum->m_before_any_store = bp_unpack_value (&bp, 1);
> }
>
> /* Read intraprocedural analysis information about NODE and all of its
> outgoing
> @@ -3420,7 +3430,8 @@ param_splitting_across_edge (cgraph_edge *cs)
> }
> else if (!ipf->safe_to_import_accesses)
> {
> - if (!all_callee_accesses_present_p (param_desc, arg_desc))
> + if (!csum->m_before_any_store
> + || !all_callee_accesses_present_p (param_desc, arg_desc))
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, " %u->%u: cannot import accesses.\n",
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr101066.c
> b/gcc/testsuite/gcc.dg/ipa/pr101066.c
> new file mode 100644
> index 00000000000..1ceb6e43136
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr101066.c
> @@ -0,0 +1,20 @@
> +/* { dg-do run } */
> +/* { dg-options "-Os -fno-ipa-cp -fno-inline" } */
> +
> +int a = 1, c, d, e;
> +int *b = &a;
> +static int g(int *h) {
> + c = *h;
> + return d;
> +}
> +static void f(int *h) {
> + e = *h;
> + *b = 0;
> + g(h);
> +}
> +int main() {
> + f(b);
> + if (c)
> + __builtin_abort();
> + return 0;
> +}
> --
> 2.31.1
