Re: [PATCH] ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066)

[Martin Jambor <mjambor@suse.cz>]

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  <mjambor@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  <mjambor@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