diff mbox series

Cleanup handling of local/readonly memory in modref and ipa-pure-const

Message ID 20200923084318.GA94622@kam.mff.cuni.cz
State New
Headers show
Series Cleanup handling of local/readonly memory in modref and ipa-pure-const | expand

Commit Message

Jan Hubicka Sept. 23, 2020, 8:43 a.m. UTC 
Hi,
this is first of cleanup patches for mod-ref interfaces.  It removes code
duplication between ipa-pure-const and ipa-modref that both wants to check
whether given memory access can interfere with memory acesses before function
call or after function return.

I pulled the logic out to refs_local_or_readonly_memory_p for references and
points_to_local_or_readonly_memory_p for pointers. It is in ipa-fnsummary.c
because I have incremental patches to track this info about function parameter
so we can ignore dereferences from parameters that are local or readonly in
modref oracle.

I am not sure this is right placement.  We could also move it to more general
place, ipa-utils or some of gimple infrastructure.
The extra includes in ipa-modref will however soon be necessary.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* ipa-fnsummary.c (refs_local_or_readonly_memory_p): New function.
	(points_to_local_or_readonly_memory_p): New function.
	* ipa-fnsummary.h (refs_local_or_readonly_memory_p,
	points_to_local_or_readonly_memory_p): Declare.
	* ipa-modref.c: Include value-range.h, ipa-prop.h and ipa-fnsummary.h
	(record_access_p): Use refs_local_or_readonly_memory_p.
	* ipa-pure-const.c (check_op): Likewise.

	* testsuite/gcc.dg/tree-ssa/local-pure-const.c: Update template.

Comments

Richard Biener Sept. 23, 2020, 9:16 a.m. UTC | #1 
On Wed, Sep 23, 2020 at 10:44 AM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> Hi,
> this is first of cleanup patches for mod-ref interfaces.  It removes code
> duplication between ipa-pure-const and ipa-modref that both wants to check
> whether given memory access can interfere with memory acesses before function
> call or after function return.
>
> I pulled the logic out to refs_local_or_readonly_memory_p for references and
> points_to_local_or_readonly_memory_p for pointers. It is in ipa-fnsummary.c
> because I have incremental patches to track this info about function parameter
> so we can ignore dereferences from parameters that are local or readonly in
> modref oracle.
>
> I am not sure this is right placement.  We could also move it to more general
> place, ipa-utils or some of gimple infrastructure.
> The extra includes in ipa-modref will however soon be necessary.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> Honza
>
>         * ipa-fnsummary.c (refs_local_or_readonly_memory_p): New function.
>         (points_to_local_or_readonly_memory_p): New function.
>         * ipa-fnsummary.h (refs_local_or_readonly_memory_p,
>         points_to_local_or_readonly_memory_p): Declare.
>         * ipa-modref.c: Include value-range.h, ipa-prop.h and ipa-fnsummary.h
>         (record_access_p): Use refs_local_or_readonly_memory_p.
>         * ipa-pure-const.c (check_op): Likewise.
>
>         * testsuite/gcc.dg/tree-ssa/local-pure-const.c: Update template.
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index 86d01addb44..ce168230105 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -2430,6 +2433,51 @@ fp_expression_p (gimple *stmt)
>    return false;
>  }
>
> +/* Return true if T references memory location that is local
> +   for the function (that means, dead after return) or read-only.  */
> +
> +bool
> +refs_local_or_readonly_memory_p (tree t)
> +{
> +  /* Non-escaping memory is fine  */
> +  t = get_base_address (t);
> +  if ((TREE_CODE (t) == MEM_REF
> +      || TREE_CODE (t) == TARGET_MEM_REF))
> +    return points_to_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
> +
> +  /* Automatic variables are fine.  */
> +  if (DECL_P (t)
> +      && auto_var_in_fn_p (t, current_function_decl))
> +    return true;
> +
> +  /* Read-only variables are fine.  */
> +  if (DECL_P (t) && TREE_READONLY (t))
> +    return true;
> +
> +  return false;
> +}
> +
> +/* Return true if T is a pointer pointing to memory location that is local
> +   for the function (that means, dead after return) or read-only.  */
> +
> +bool
> +points_to_local_or_readonly_memory_p (tree t)
> +{
> +  /*if (!POINTER_TYPE_P (TREE_TYPE (t)))
> +    return false; */

remove ^^^

> +  STRIP_NOPS (t);

This wasn't in the original code - did you really run into (long)&x or what?

> +  /* See if memory location is clearly invalid.  */
> +  if (TREE_CODE (t) == INTEGER_CST)
> +    return flag_delete_null_pointer_checks;
> +  if (TREE_CODE (t) == SSA_NAME)
> +    return SSA_NAME_POINTS_TO_READONLY_MEMORY (t)
> +          || !ptr_deref_may_alias_global_p (t);
> +  if (TREE_CODE (t) == ADDR_EXPR)
> +    return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
> +  return false;
> +}
> +
> +
>  /* Analyze function body for NODE.
>     EARLY indicates run from early optimization pipeline.  */
>
> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
> index c6ddc9f3199..a3223d06acc 100644
> --- a/gcc/ipa-fnsummary.h
> +++ b/gcc/ipa-fnsummary.h
> @@ -357,6 +357,8 @@ void estimate_ipcp_clone_size_and_time (struct cgraph_node *,
>  void ipa_merge_fn_summary_after_inlining (struct cgraph_edge *edge);
>  void ipa_update_overall_fn_summary (struct cgraph_node *node, bool reset = true);
>  void compute_fn_summary (struct cgraph_node *, bool);
> +bool refs_local_or_readonly_memory_p (tree);
> +bool points_to_local_or_readonly_memory_p (tree);
>
>
>  void evaluate_properties_for_edge (struct cgraph_edge *e,
> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
> index af0b710333e..d1b091ae29e 100644
> --- a/gcc/ipa-modref.c
> +++ b/gcc/ipa-modref.c
> @@ -62,6 +62,9 @@ along with GCC; see the file COPYING3.  If not see
>  #include "calls.h"
>  #include "ipa-modref-tree.h"
>  #include "ipa-modref.h"
> +#include "value-range.h"
> +#include "ipa-prop.h"
> +#include "ipa-fnsummary.h"
>
>  /* Class (from which there is one global instance) that holds modref summaries
>     for all analyzed functions.  */
> @@ -317,36 +415,12 @@ record_access_lto (modref_records_lto *tt, ao_ref *ref)
>  static bool
>  record_access_p (tree expr)
>  {
> -  /* Non-escaping memory is fine  */
> -  tree t = get_base_address (expr);
> -  if (t && (INDIRECT_REF_P (t)
> -           || TREE_CODE (t) == MEM_REF
> -           || TREE_CODE (t) == TARGET_MEM_REF)
> -       && TREE_CODE (TREE_OPERAND (t, 0)) == SSA_NAME
> -       && !ptr_deref_may_alias_global_p (TREE_OPERAND (t, 0)))
> +  if (refs_local_or_readonly_memory_p (expr))
>      {
>        if (dump_file)
> -       fprintf (dump_file, "   - Non-escaping memory, ignoring.\n");
> +       fprintf (dump_file, "   - Read-only or local, ignoring.\n");
>        return false;
>      }
> -
> -  /* Automatic variables are fine.  */
> -  if (DECL_P (t)
> -      && auto_var_in_fn_p (t, current_function_decl))
> -    {
> -      if (dump_file)
> -       fprintf (dump_file, "   - Automatic variable, ignoring.\n");
> -      return false;
> -    }
> -
> -  /* Read-only variables are fine.  */
> -  if (DECL_P (t) && TREE_READONLY (t))
> -    {
> -      if (dump_file)
> -       fprintf (dump_file, "   - Read-only variable, ignoring.\n");
> -      return false;
> -    }
> -
>    return true;
>  }
>
> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
> index 564c6629c92..bdbccd010dc 100644
> --- a/gcc/ipa-pure-const.c
> +++ b/gcc/ipa-pure-const.c
> @@ -381,13 +381,11 @@ check_op (funct_state local, tree t, bool checking_write)
>         fprintf (dump_file, "    Volatile indirect ref is not const/pure\n");
>        return;
>      }
> -  else if (t
> -          && (INDIRECT_REF_P (t) || TREE_CODE (t) == MEM_REF)
> -          && TREE_CODE (TREE_OPERAND (t, 0)) == SSA_NAME
> -          && !ptr_deref_may_alias_global_p (TREE_OPERAND (t, 0)))
> +  else if (refs_local_or_readonly_memory_p (t))
>      {
>        if (dump_file)
> -       fprintf (dump_file, "    Indirect ref to local memory is OK\n");
> +       fprintf (dump_file, "    Indirect ref to local or readonly "
> +                "memory is OK\n");
>        return;
>      }
>    else if (checking_write)
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c b/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c
> index 3c358e01393..6746758ca88 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c
> @@ -12,5 +12,5 @@ t(int a, int b, int c)
>      p = &c;
>    return *p;
>  }
> -/* { dg-final { scan-tree-dump-times "local memory is OK" 1 "local-pure-const1"} } */
> +/* { dg-final { scan-tree-dump-times "local or readonly memory is OK" 1 "local-pure-const1"} } */
>  /* { dg-final { scan-tree-dump-times "found to be const" 1 "local-pure-const1"} } */
Jan Hubicka Sept. 23, 2020, 9:55 a.m. UTC | #2 
> > +/* Return true if T is a pointer pointing to memory location that is local
> > +   for the function (that means, dead after return) or read-only.  */
> > +
> > +bool
> > +points_to_local_or_readonly_memory_p (tree t)
> > +{
> > +  /*if (!POINTER_TYPE_P (TREE_TYPE (t)))
> > +    return false; */
> 
> remove ^^^
Ahh, sorry.
> 
> > +  STRIP_NOPS (t);
> 
> This wasn't in the original code - did you really run into (long)&x or what?

Old code works on references only.  In the followup I want to run it on
function arguments, so we can propagate info that i.e. this pointer
passed to function points to automatic variable.

I think I can see nops in the gimple args so I should strip it?
I did not verify if that happens in pratice.

Honza
Richard Biener Sept. 23, 2020, 10:20 a.m. UTC | #3 
On Wed, Sep 23, 2020 at 11:55 AM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> > > +/* Return true if T is a pointer pointing to memory location that is local
> > > +   for the function (that means, dead after return) or read-only.  */
> > > +
> > > +bool
> > > +points_to_local_or_readonly_memory_p (tree t)
> > > +{
> > > +  /*if (!POINTER_TYPE_P (TREE_TYPE (t)))
> > > +    return false; */
> >
> > remove ^^^
> Ahh, sorry.
> >
> > > +  STRIP_NOPS (t);
> >
> > This wasn't in the original code - did you really run into (long)&x or what?
>
> Old code works on references only.  In the followup I want to run it on
> function arguments, so we can propagate info that i.e. this pointer
> passed to function points to automatic variable.
>
> I think I can see nops in the gimple args so I should strip it?

No, there cannot be conversions in gimple args.

> I did not verify if that happens in pratice.
>
> Honza
Jan Hubicka Sept. 23, 2020, 12:32 p.m. UTC | #4 
> On Wed, Sep 23, 2020 at 11:55 AM Jan Hubicka <hubicka@ucw.cz> wrote:
> >
> > > > +/* Return true if T is a pointer pointing to memory location that is local
> > > > +   for the function (that means, dead after return) or read-only.  */
> > > > +
> > > > +bool
> > > > +points_to_local_or_readonly_memory_p (tree t)
> > > > +{
> > > > +  /*if (!POINTER_TYPE_P (TREE_TYPE (t)))
> > > > +    return false; */
> > >
> > > remove ^^^
> > Ahh, sorry.
> > >
> > > > +  STRIP_NOPS (t);
> > >
> > > This wasn't in the original code - did you really run into (long)&x or what?
> >
> > Old code works on references only.  In the followup I want to run it on
> > function arguments, so we can propagate info that i.e. this pointer
> > passed to function points to automatic variable.
> >
> > I think I can see nops in the gimple args so I should strip it?
> 
> No, there cannot be conversions in gimple args.

I will re-test with the strip dropped. OK with that change?

Honza
> 
> > I did not verify if that happens in pratice.
> >
> > Honza
Richard Biener Sept. 23, 2020, 12:35 p.m. UTC | #5 
On Wed, Sep 23, 2020 at 2:32 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> > On Wed, Sep 23, 2020 at 11:55 AM Jan Hubicka <hubicka@ucw.cz> wrote:
> > >
> > > > > +/* Return true if T is a pointer pointing to memory location that is local
> > > > > +   for the function (that means, dead after return) or read-only.  */
> > > > > +
> > > > > +bool
> > > > > +points_to_local_or_readonly_memory_p (tree t)
> > > > > +{
> > > > > +  /*if (!POINTER_TYPE_P (TREE_TYPE (t)))
> > > > > +    return false; */
> > > >
> > > > remove ^^^
> > > Ahh, sorry.
> > > >
> > > > > +  STRIP_NOPS (t);
> > > >
> > > > This wasn't in the original code - did you really run into (long)&x or what?
> > >
> > > Old code works on references only.  In the followup I want to run it on
> > > function arguments, so we can propagate info that i.e. this pointer
> > > passed to function points to automatic variable.
> > >
> > > I think I can see nops in the gimple args so I should strip it?
> >
> > No, there cannot be conversions in gimple args.
>
> I will re-test with the strip dropped. OK with that change?

Yes (and the commented check removed)

Richard.

> Honza
> >
> > > I did not verify if that happens in pratice.
> > >
> > > Honza
diff mbox series

Patch

diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 86d01addb44..ce168230105 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -2430,6 +2433,51 @@  fp_expression_p (gimple *stmt)
   return false;
 }
 
+/* Return true if T references memory location that is local
+   for the function (that means, dead after return) or read-only.  */
+
+bool
+refs_local_or_readonly_memory_p (tree t)
+{
+  /* Non-escaping memory is fine  */
+  t = get_base_address (t);
+  if ((TREE_CODE (t) == MEM_REF
+      || TREE_CODE (t) == TARGET_MEM_REF))
+    return points_to_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
+
+  /* Automatic variables are fine.  */
+  if (DECL_P (t)
+      && auto_var_in_fn_p (t, current_function_decl))
+    return true;
+
+  /* Read-only variables are fine.  */
+  if (DECL_P (t) && TREE_READONLY (t))
+    return true;
+
+  return false;
+}
+
+/* Return true if T is a pointer pointing to memory location that is local
+   for the function (that means, dead after return) or read-only.  */
+
+bool
+points_to_local_or_readonly_memory_p (tree t)
+{
+  /*if (!POINTER_TYPE_P (TREE_TYPE (t)))
+    return false; */
+  STRIP_NOPS (t);
+  /* See if memory location is clearly invalid.  */
+  if (TREE_CODE (t) == INTEGER_CST)
+    return flag_delete_null_pointer_checks;
+  if (TREE_CODE (t) == SSA_NAME)
+    return SSA_NAME_POINTS_TO_READONLY_MEMORY (t)
+	   || !ptr_deref_may_alias_global_p (t);
+  if (TREE_CODE (t) == ADDR_EXPR)
+    return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
+  return false;
+}
+
+
 /* Analyze function body for NODE.
    EARLY indicates run from early optimization pipeline.  */
 
diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
index c6ddc9f3199..a3223d06acc 100644
--- a/gcc/ipa-fnsummary.h
+++ b/gcc/ipa-fnsummary.h
@@ -357,6 +357,8 @@  void estimate_ipcp_clone_size_and_time (struct cgraph_node *,
 void ipa_merge_fn_summary_after_inlining (struct cgraph_edge *edge);
 void ipa_update_overall_fn_summary (struct cgraph_node *node, bool reset = true);
 void compute_fn_summary (struct cgraph_node *, bool);
+bool refs_local_or_readonly_memory_p (tree);
+bool points_to_local_or_readonly_memory_p (tree);
 
 
 void evaluate_properties_for_edge (struct cgraph_edge *e,
diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
index af0b710333e..d1b091ae29e 100644
--- a/gcc/ipa-modref.c
+++ b/gcc/ipa-modref.c
@@ -62,6 +62,9 @@  along with GCC; see the file COPYING3.  If not see
 #include "calls.h"
 #include "ipa-modref-tree.h"
 #include "ipa-modref.h"
+#include "value-range.h"
+#include "ipa-prop.h"
+#include "ipa-fnsummary.h"
 
 /* Class (from which there is one global instance) that holds modref summaries
    for all analyzed functions.  */
@@ -317,36 +415,12 @@  record_access_lto (modref_records_lto *tt, ao_ref *ref)
 static bool
 record_access_p (tree expr)
 {
-  /* Non-escaping memory is fine  */
-  tree t = get_base_address (expr);
-  if (t && (INDIRECT_REF_P (t)
-	    || TREE_CODE (t) == MEM_REF
-	    || TREE_CODE (t) == TARGET_MEM_REF)
-	&& TREE_CODE (TREE_OPERAND (t, 0)) == SSA_NAME
-	&& !ptr_deref_may_alias_global_p (TREE_OPERAND (t, 0)))
+  if (refs_local_or_readonly_memory_p (expr))
     {
       if (dump_file)
-	fprintf (dump_file, "   - Non-escaping memory, ignoring.\n");
+	fprintf (dump_file, "   - Read-only or local, ignoring.\n");
       return false;
     }
-
-  /* Automatic variables are fine.  */
-  if (DECL_P (t)
-      && auto_var_in_fn_p (t, current_function_decl))
-    {
-      if (dump_file)
-	fprintf (dump_file, "   - Automatic variable, ignoring.\n");
-      return false;
-    }
-
-  /* Read-only variables are fine.  */
-  if (DECL_P (t) && TREE_READONLY (t))
-    {
-      if (dump_file)
-	fprintf (dump_file, "   - Read-only variable, ignoring.\n");
-      return false;
-    }
-
   return true;
 }
 
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 564c6629c92..bdbccd010dc 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -381,13 +381,11 @@  check_op (funct_state local, tree t, bool checking_write)
 	fprintf (dump_file, "    Volatile indirect ref is not const/pure\n");
       return;
     }
-  else if (t
-  	   && (INDIRECT_REF_P (t) || TREE_CODE (t) == MEM_REF)
-	   && TREE_CODE (TREE_OPERAND (t, 0)) == SSA_NAME
-	   && !ptr_deref_may_alias_global_p (TREE_OPERAND (t, 0)))
+  else if (refs_local_or_readonly_memory_p (t))
     {
       if (dump_file)
-	fprintf (dump_file, "    Indirect ref to local memory is OK\n");
+	fprintf (dump_file, "    Indirect ref to local or readonly "
+		 "memory is OK\n");
       return;
     }
   else if (checking_write)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c b/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c
index 3c358e01393..6746758ca88 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/local-pure-const.c
@@ -12,5 +12,5 @@  t(int a, int b, int c)
     p = &c;
   return *p;
 }
-/* { dg-final { scan-tree-dump-times "local memory is OK" 1 "local-pure-const1"} } */
+/* { dg-final { scan-tree-dump-times "local or readonly memory is OK" 1 "local-pure-const1"} } */
 /* { dg-final { scan-tree-dump-times "found to be const" 1 "local-pure-const1"} } */