diff mbox

Set nonnull attribute to ptr_info_def based on VRP

Message ID 8c2fd9cf-2504-387a-04e5-5ba4db2d87f3@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Oct. 13, 2016, 11:12 p.m. UTC 
Hi Richard,

On 13/10/16 20:44, Richard Biener wrote:
> On Thu, Oct 13, 2016 at 6:49 AM, kugan
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi Richard,
>>
>>>
>>> what does this try to do?  Preserve info VRP computed across PTA?
>>>
>>> I think we didn't yet sort out the nonlocal/escaped vs. null handling
>>> properly
>>> (or how PTA should handle get_ptr_nonnull).  The way you are using it
>>> asks for pt.null to be orthogonal to nonlocal/escaped and thus having
>>> nonlocal or escaped would also require setting ptr.null in PTA.  It then
>>> would be also more canonical to set it for pt.anything as well.  Which
>>> means conservatively handling it would be equivalent to flipping its
>>> semantic and changing its name to pt.nonnull.
>>>
>>> That said, you seem to be simply "reserving" the bit for VRP, keeping it
>>> conservatively true when "not computed".  I guess I'm fine with this for
>>> now
>>> but it should be documented in the header file that way.
>>>
>>
>> Thanks for the comments.
>>
>> To summarize, currently I am not relying on PTA analysis at all. Just saving
>> null from VRP (or rather nonnull) and preserving it across PTA. Primary
>> intention is to pass it for PARM_DECL SSA names (from ipa-vrp).
>>
>> In this case, using  pt.anything/nonlocal/escaped will only make the result
>> more pessimistic.
>>
>> Ideally, we should improve pt.null within PTA but for now as you said, I
>> will document it.
>>
>> When we start using pt.null from PTA analysis, we would also have to take
>> into account pt.anything/nonlocal/escaped.
>>
>> Does that make sense?
>
> Yes.


Here is the revised patch based on the review. I also had to adjust two 
testcases since we set pt.null conservatively and dumps that too.

Thanks,
Kugan

gcc/ChangeLog:

2016-10-14  Kugan Vivekanandarajah  <kuganv@linaro.org>

	* tree-ssa-alias.h (pt_solution_singleton_or_null_p): Renamed from
	pt_solution_singleton_p.
	* tree-ssa-ccp.c (fold_builtin_alloca_with_align): Use renamed
	pt_solution_singleton_or_null_p from pt_solution_singleton_p.
	* tree-ssa-structalias.c (find_what_var_points_to): Conservatively set
	pt.null to 1.
	(find_what_p_points_to): Preserve pointer nonnull computed by VRP.
	(pt_solution_singleton_or_null_p): Renamed from
	pt_solution_singleton_p.
	* tree-ssanames.h (set_ptr_nonnull): Declare.
	(get_ptr_nonnull): Likewise.
	* tree-ssanames.c (set_ptr_nonnull): New.
	(get_ptr_nonnull): Likewise.
	* tree-vrp.c (vrp_finalize): Set ptr that are nonnull.
	(evrp_dom_walker::before_dom_children): Likewise.


gcc/testsuite/ChangeLog:

2016-10-14  Kugan Vivekanandarajah  <kuganv@linaro.org>

	* gcc.dg/torture/pr39074-2.c: Adjust testcase.
	* gcc.dg/torture/pr39074.c: Likewise.

Comments

Richard Biener Oct. 14, 2016, 12:53 p.m. UTC | #1 
On Fri, Oct 14, 2016 at 1:12 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>
> On 13/10/16 20:44, Richard Biener wrote:
>>
>> On Thu, Oct 13, 2016 at 6:49 AM, kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>> Hi Richard,
>>>
>>>>
>>>> what does this try to do?  Preserve info VRP computed across PTA?
>>>>
>>>> I think we didn't yet sort out the nonlocal/escaped vs. null handling
>>>> properly
>>>> (or how PTA should handle get_ptr_nonnull).  The way you are using it
>>>> asks for pt.null to be orthogonal to nonlocal/escaped and thus having
>>>> nonlocal or escaped would also require setting ptr.null in PTA.  It then
>>>> would be also more canonical to set it for pt.anything as well.  Which
>>>> means conservatively handling it would be equivalent to flipping its
>>>> semantic and changing its name to pt.nonnull.
>>>>
>>>> That said, you seem to be simply "reserving" the bit for VRP, keeping it
>>>> conservatively true when "not computed".  I guess I'm fine with this for
>>>> now
>>>> but it should be documented in the header file that way.
>>>>
>>>
>>> Thanks for the comments.
>>>
>>> To summarize, currently I am not relying on PTA analysis at all. Just
>>> saving
>>> null from VRP (or rather nonnull) and preserving it across PTA. Primary
>>> intention is to pass it for PARM_DECL SSA names (from ipa-vrp).
>>>
>>> In this case, using  pt.anything/nonlocal/escaped will only make the
>>> result
>>> more pessimistic.
>>>
>>> Ideally, we should improve pt.null within PTA but for now as you said, I
>>> will document it.
>>>
>>> When we start using pt.null from PTA analysis, we would also have to take
>>> into account pt.anything/nonlocal/escaped.
>>>
>>> Does that make sense?
>>
>>
>> Yes.
>
>
>
> Here is the revised patch based on the review. I also had to adjust two
> testcases since we set pt.null conservatively and dumps that too.

Hmm, sorry for another request, but seeing the testcase change I rather
want to expose the conservatism only at find_what_p_points_to time.

Thus,

@@ -6382,6 +6382,8 @@ find_what_var_points_to (tree fndecl, varinfo_t orig_vi)

   *slot = pt = XOBNEW (&final_solutions_obstack, struct pt_solution);
   memset (pt, 0, sizeof (struct pt_solution));
+  /* Conservatively set to NULL from PTA (to true). */
+  pt->null = 1;

   /* Translate artificial variables into SSA_NAME_PTR_INFO
      attributes.  */

remove this hunk

@@ -6466,6 +6469,10 @@ find_what_p_points_to (tree fndecl, tree p)

   pi = get_ptr_info (p);
   pi->pt = find_what_var_points_to (fndecl, vi);
+  /* Preserve pointer nonnull computed by VRP.  See get_ptr_nonnull
+     in gcc/tree-ssaname.c for more information.  */
+  if (nonnull)
+    set_ptr_nonnull (p);

and add pi->pt.null = true; here (with the comment from above).  This
preserves the (possibly incorrect) PTA solution in the dumps but does
not leak it to SSA_NAME_PTR_INFO.

+bool
+get_ptr_nonnull (const_tree name)
+{
+  gcc_assert (POINTER_TYPE_P (TREE_TYPE (name)));
+  struct ptr_info_def *pi = SSA_NAME_PTR_INFO (name);
+  if (pi == NULL)
+    return false;
+  /* TODO Now pt->null is conservatively set to null in PTA

set to true

+     analysis. vrp is the only pass (including ipa-vrp)
+     that clears pt.null via set_ptr_nonull when it knows
+     for sure. PTA will preserves the pt.null value set by VRP.
+

Ok with those changes.

Thanks,
Richard.


> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2016-10-14  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         * tree-ssa-alias.h (pt_solution_singleton_or_null_p): Renamed from
>         pt_solution_singleton_p.
>         * tree-ssa-ccp.c (fold_builtin_alloca_with_align): Use renamed
>         pt_solution_singleton_or_null_p from pt_solution_singleton_p.
>         * tree-ssa-structalias.c (find_what_var_points_to): Conservatively
> set
>         pt.null to 1.
>         (find_what_p_points_to): Preserve pointer nonnull computed by VRP.
>         (pt_solution_singleton_or_null_p): Renamed from
>         pt_solution_singleton_p.
>         * tree-ssanames.h (set_ptr_nonnull): Declare.
>         (get_ptr_nonnull): Likewise.
>         * tree-ssanames.c (set_ptr_nonnull): New.
>         (get_ptr_nonnull): Likewise.
>         * tree-vrp.c (vrp_finalize): Set ptr that are nonnull.
>         (evrp_dom_walker::before_dom_children): Likewise.
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-10-14  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         * gcc.dg/torture/pr39074-2.c: Adjust testcase.
>         * gcc.dg/torture/pr39074.c: Likewise.
diff mbox

Patch

From 0bc1c2efb600854148889bfdd9d121a3edc2841b Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Date: Wed, 12 Oct 2016 13:48:07 +1100
Subject: [PATCH 1/3] Add-nonnull-to-pointer-from-VRP

---
 gcc/testsuite/gcc.dg/torture/pr39074-2.c |  2 +-
 gcc/testsuite/gcc.dg/torture/pr39074.c   |  2 +-
 gcc/tree-ssa-alias.h                     |  2 +-
 gcc/tree-ssa-ccp.c                       |  2 +-
 gcc/tree-ssa-structalias.c               | 11 ++++++--
 gcc/tree-ssanames.c                      | 29 +++++++++++++++++++++
 gcc/tree-ssanames.h                      |  2 ++
 gcc/tree-vrp.c                           | 44 +++++++++++++++++++++-----------
 8 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/torture/pr39074-2.c b/gcc/testsuite/gcc.dg/torture/pr39074-2.c
index 740b463..0693f2d 100644
--- a/gcc/testsuite/gcc.dg/torture/pr39074-2.c
+++ b/gcc/testsuite/gcc.dg/torture/pr39074-2.c
@@ -31,4 +31,4 @@  int main()
 }
 
 /* { dg-final { scan-tree-dump "y.._. = { i }" "alias" } } */
-/* { dg-final { scan-tree-dump "y.._., points-to vars: { D..... }" "alias" } } */
+/* { dg-final { scan-tree-dump "y.._., points-to NULL, points-to vars: { D..... }" "alias" } } */
diff --git a/gcc/testsuite/gcc.dg/torture/pr39074.c b/gcc/testsuite/gcc.dg/torture/pr39074.c
index 31ed499..54c444e 100644
--- a/gcc/testsuite/gcc.dg/torture/pr39074.c
+++ b/gcc/testsuite/gcc.dg/torture/pr39074.c
@@ -30,4 +30,4 @@  int main()
 }
 
 /* { dg-final { scan-tree-dump "y.._. = { i }" "alias" } } */
-/* { dg-final { scan-tree-dump "y.._., points-to vars: { D..... }" "alias" } } */
+/* { dg-final { scan-tree-dump "y.._., points-to NULL, points-to vars: { D..... }" "alias" } } */
diff --git a/gcc/tree-ssa-alias.h b/gcc/tree-ssa-alias.h
index 6680cc0..27a06fc 100644
--- a/gcc/tree-ssa-alias.h
+++ b/gcc/tree-ssa-alias.h
@@ -146,7 +146,7 @@  extern void dump_alias_stats (FILE *);
 /* In tree-ssa-structalias.c  */
 extern unsigned int compute_may_aliases (void);
 extern bool pt_solution_empty_p (struct pt_solution *);
-extern bool pt_solution_singleton_p (struct pt_solution *, unsigned *);
+extern bool pt_solution_singleton_or_null_p (struct pt_solution *, unsigned *);
 extern bool pt_solution_includes_global (struct pt_solution *);
 extern bool pt_solution_includes (struct pt_solution *, const_tree);
 extern bool pt_solutions_intersect (struct pt_solution *, struct pt_solution *);
diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index fe9a313..61754d8 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -2135,7 +2135,7 @@  fold_builtin_alloca_with_align (gimple *stmt)
       {
 	bool singleton_p;
 	unsigned uid;
-	singleton_p = pt_solution_singleton_p (&pi->pt, &uid);
+	singleton_p = pt_solution_singleton_or_null_p (&pi->pt, &uid);
 	gcc_assert (singleton_p);
 	SET_DECL_PT_UID (var, uid);
       }
diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 2a4ab2f..ad65c94 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -6382,6 +6382,8 @@  find_what_var_points_to (tree fndecl, varinfo_t orig_vi)
 
   *slot = pt = XOBNEW (&final_solutions_obstack, struct pt_solution);
   memset (pt, 0, sizeof (struct pt_solution));
+  /* Conservatively set to NULL from PTA (to true). */
+  pt->null = 1;
 
   /* Translate artificial variables into SSA_NAME_PTR_INFO
      attributes.  */
@@ -6451,6 +6453,7 @@  find_what_p_points_to (tree fndecl, tree p)
   struct ptr_info_def *pi;
   tree lookup_p = p;
   varinfo_t vi;
+  bool nonnull = get_ptr_nonnull (p);
 
   /* For parameters, get at the points-to set for the actual parm
      decl.  */
@@ -6466,6 +6469,10 @@  find_what_p_points_to (tree fndecl, tree p)
 
   pi = get_ptr_info (p);
   pi->pt = find_what_var_points_to (fndecl, vi);
+  /* Preserve pointer nonnull computed by VRP.  See get_ptr_nonnull
+     in gcc/tree-ssaname.c for more information.  */
+  if (nonnull)
+    set_ptr_nonnull (p);
 }
 
 
@@ -6599,10 +6606,10 @@  pt_solution_empty_p (struct pt_solution *pt)
    return the var uid in *UID.  */
 
 bool
-pt_solution_singleton_p (struct pt_solution *pt, unsigned *uid)
+pt_solution_singleton_or_null_p (struct pt_solution *pt, unsigned *uid)
 {
   if (pt->anything || pt->nonlocal || pt->escaped || pt->ipa_escaped
-      || pt->null || pt->vars == NULL
+      || pt->vars == NULL
       || !bitmap_single_bit_set_p (pt->vars))
     return false;
 
diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c
index 64ab13a..fe6a0f7 100644
--- a/gcc/tree-ssanames.c
+++ b/gcc/tree-ssanames.c
@@ -374,6 +374,35 @@  get_range_info (const_tree name, wide_int *min, wide_int *max)
   return SSA_NAME_RANGE_TYPE (name);
 }
 
+/* Set nonnull attribute to pointer NAME.  */
+
+void
+set_ptr_nonnull (tree name)
+{
+  gcc_assert (POINTER_TYPE_P (TREE_TYPE (name)));
+  struct ptr_info_def *pi = get_ptr_info (name);
+  pi->pt.null = 0;
+}
+
+/* Return nonnull attribute of pointer NAME.  */
+bool
+get_ptr_nonnull (const_tree name)
+{
+  gcc_assert (POINTER_TYPE_P (TREE_TYPE (name)));
+  struct ptr_info_def *pi = SSA_NAME_PTR_INFO (name);
+  if (pi == NULL)
+    return false;
+  /* TODO Now pt->null is conservatively set to null in PTA
+     analysis. vrp is the only pass (including ipa-vrp)
+     that clears pt.null via set_ptr_nonull when it knows
+     for sure. PTA will preserves the pt.null value set by VRP.
+
+     When PTA analysis is improved, pt.anything, pt.nonlocal
+     and pt.escaped may also has to be considered before
+     deciding that pointer cannot point to NULL.  */
+  return !pi->pt.null;
+}
+
 /* Change non-zero bits bitmask of NAME.  */
 
 void
diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
index 4496e1d..d39cc9d 100644
--- a/gcc/tree-ssanames.h
+++ b/gcc/tree-ssanames.h
@@ -88,6 +88,8 @@  extern void set_ptr_info_alignment (struct ptr_info_def *, unsigned int,
 extern void adjust_ptr_info_misalignment (struct ptr_info_def *,
 					  unsigned int);
 extern struct ptr_info_def *get_ptr_info (tree);
+extern void set_ptr_nonnull (tree);
+extern bool get_ptr_nonnull (const_tree);
 
 extern tree copy_ssa_name_fn (struct function *, tree, gimple *);
 extern void duplicate_ssa_name_ptr_info (tree, struct ptr_info_def *);
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 8a129c6..7e4f947 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -10603,18 +10603,24 @@  vrp_finalize (bool warn_array_bounds_p)
       {
 	tree name = ssa_name (i);
 
-      if (!name
-	  || POINTER_TYPE_P (TREE_TYPE (name))
-	  || (vr_value[i]->type == VR_VARYING)
-	  || (vr_value[i]->type == VR_UNDEFINED))
-	continue;
+	if (!name
+	    || (vr_value[i]->type == VR_VARYING)
+	    || (vr_value[i]->type == VR_UNDEFINED)
+	    || (TREE_CODE (vr_value[i]->min) != INTEGER_CST)
+	    || (TREE_CODE (vr_value[i]->max) != INTEGER_CST))
+	  continue;
 
-      if ((TREE_CODE (vr_value[i]->min) == INTEGER_CST)
-	  && (TREE_CODE (vr_value[i]->max) == INTEGER_CST)
-	  && (vr_value[i]->type == VR_RANGE
-	      || vr_value[i]->type == VR_ANTI_RANGE))
-	set_range_info (name, vr_value[i]->type, vr_value[i]->min,
-			vr_value[i]->max);
+	if (POINTER_TYPE_P (TREE_TYPE (name))
+	    && ((vr_value[i]->type == VR_RANGE
+		 && range_includes_zero_p (vr_value[i]->min,
+					   vr_value[i]->max) == 0)
+		|| (vr_value[i]->type == VR_ANTI_RANGE
+		    && range_includes_zero_p (vr_value[i]->min,
+					      vr_value[i]->max) == 1)))
+	  set_ptr_nonnull (name);
+	else if (!POINTER_TYPE_P (TREE_TYPE (name)))
+	  set_range_info (name, vr_value[i]->type, vr_value[i]->min,
+			  vr_value[i]->max);
       }
 
   substitute_and_fold (op_with_constant_singleton_value_range,
@@ -10817,17 +10823,25 @@  evrp_dom_walker::before_dom_children (basic_block bb)
 	  def_operand_p def_p = SINGLE_SSA_DEF_OPERAND (stmt, SSA_OP_DEF);
 	  /* Set the SSA with the value range.  */
 	  if (def_p
-	      && TREE_CODE (DEF_FROM_PTR (def_p)) == SSA_NAME
-	      && INTEGRAL_TYPE_P (TREE_TYPE (DEF_FROM_PTR (def_p))))
+	      && TREE_CODE (DEF_FROM_PTR (def_p)) == SSA_NAME)
 	    {
 	      tree def = DEF_FROM_PTR (def_p);
 	      value_range *vr = get_value_range (def);
 
-	      if ((vr->type == VR_RANGE
-		   || vr->type == VR_ANTI_RANGE)
+	      if (INTEGRAL_TYPE_P (TREE_TYPE (DEF_FROM_PTR (def_p)))
+		  && (vr->type == VR_RANGE
+		      || vr->type == VR_ANTI_RANGE)
 		  && (TREE_CODE (vr->min) == INTEGER_CST)
 		  && (TREE_CODE (vr->max) == INTEGER_CST))
 		set_range_info (def, vr->type, vr->min, vr->max);
+	      else if (POINTER_TYPE_P (TREE_TYPE (DEF_FROM_PTR (def_p)))
+		       && ((vr->type == VR_RANGE
+			    && range_includes_zero_p (vr->min,
+						      vr->max) == 0)
+			   || (vr->type == VR_ANTI_RANGE
+			       && range_includes_zero_p (vr->min,
+							 vr->max) == 1)))
+		set_ptr_nonnull (def);
 	    }
 	}
       else
-- 
2.7.4