public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
From: Martin Jambor <jamborm@gcc.gnu.org>
To: gcc-cvs@gcc.gnu.org
Subject: [gcc r12-2172] ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066)
Date: Thu,  8 Jul 2021 17:46:15 +0000 (GMT)	[thread overview]
Message-ID: <20210708174615.A80083AA9424@sourceware.org> (raw)

https://gcc.gnu.org/g:763121ccd908f52bc666f277ea2cf42110b3aad9

commit r12-2172-g763121ccd908f52bc666f277ea2cf42110b3aad9
Author: Martin Jambor <mjambor@suse.cz>
Date:   Thu Jul 8 19:44:41 2021 +0200

    ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066)
    
    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.
    
    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.

Diff:
---
 gcc/ipa-sra.c                       | 15 +++++++++++++--
 gcc/testsuite/gcc.dg/ipa/pr101066.c | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

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;
+}


                 reply	other threads:[~2021-07-08 17:46 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210708174615.A80083AA9424@sourceware.org \
    --to=jamborm@gcc.gnu.org \
    --cc=gcc-cvs@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).