diff mbox

Set nonnull attribute to ptr_info_def based on VRP

Message ID 3b8ae8ba-97e7-7363-fecc-bd0e0f85abcf@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Oct. 7, 2016, 12:53 a.m. UTC 
Hi Richard,

Thanks for the review.

On 09/08/16 18:58, Richard Biener wrote:
> On Tue, Aug 9, 2016 at 12:58 AM, kugan
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi Jakub,
>>
>> Thanks for the review.
>>
>> On 08/08/16 16:40, Jakub Jelinek wrote:
>>>
>>> On Mon, Aug 08, 2016 at 01:36:51PM +1000, kugan wrote:
>>>>
>>>> diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
>>>> index c81b1a1..6e34433 100644
>>>> --- a/gcc/tree-ssanames.h
>>>> +++ b/gcc/tree-ssanames.h
>>>> @@ -43,6 +43,9 @@ struct GTY(()) ptr_info_def
>>>>       above alignment.  Access only through the same helper functions as
>>>> align
>>>>       above.  */
>>>>    unsigned int misalign;
>>>> +  /* When this pointer is knonw to be nnonnull this would be true
>>>> otherwise
>>>> +     false.  */
>>>> +  bool  nonnull_p;
>>>>  };
>>>
>>>
>>> Why do you need this?  Doesn't the pt.null bit represent that already?
>>
>>
>> It looks like I can use this. As in gcc/tree-ssa-alias.h:
>>
>>   /* Nonzero if the points-to set includes 'nothing', the points-to set
>>      includes memory at address NULL.  */
>>   unsigned int null : 1;
>>
>> But in gcc/tree-ssa-alias.c, ptrs_compare_unequal has the following comment
>> which says:
>>
>> /* ???  We'd like to handle ptr1 != NULL and ptr1 != ptr2
>>      but those require pt.null to be conservatively correct.  */
>>
>> Does that means it can be wrong at times? I haven't looked it in detail yet
>> but if it is, it would be a problem.
>
> Yes, this bit is not correctly computed for all cases (well, it works
> for trivial
> ones but for example misses weaks and maybe some other corner-cases).
>
> Currently there are no consumers of this bit so the incorrectness
> doesn't matter.
>
> I suggest you simply use that bit but set it conservatively from PTA (to true)
> for now and adjust it from VRP only.
>
> I do have some patches for fixinig some of the .nonnull_p issues in PTA but
> they come at a cost of more constraints.

Here is a version of the patch that does this. I have a follow up patch 
to use this in ipa-vrp. I will send it for review once this is OK.

Bootstrapped and regression tested on x86_64-linux-gnu with no new 
regressions. Is this OK for trunk?


Thanks,
Kugan

gcc/ChangeLog:

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

	* tree-ssa-alias.c (dump_points_to_solution): Dump non null as it
	  is set to null conservatively.
	* tree-ssa-structalias.c (find_what_var_points_to): Set to null
	  conservatively.
	(pt_solution_singleton_p): Dont check null.
	* 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.
>
> Richard.
>
>>> Also, formatting and spelling:
>>> s/knonw/known/
>>> s/nnon/non/
>>> s/bool  /bool /
>>
>>
>> I will change this.
>>
>> Thanks,
>> Kugan
>>>
>>>
>>>         Jakub
>>>
>>

Comments

Richard Biener Oct. 7, 2016, 10:03 a.m. UTC | #1 
On Fri, Oct 7, 2016 at 2:53 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
> Thanks for the review.
>
>
> On 09/08/16 18:58, Richard Biener wrote:
>>
>> On Tue, Aug 9, 2016 at 12:58 AM, kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>> Hi Jakub,
>>>
>>> Thanks for the review.
>>>
>>> On 08/08/16 16:40, Jakub Jelinek wrote:
>>>>
>>>>
>>>> On Mon, Aug 08, 2016 at 01:36:51PM +1000, kugan wrote:
>>>>>
>>>>>
>>>>> diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
>>>>> index c81b1a1..6e34433 100644
>>>>> --- a/gcc/tree-ssanames.h
>>>>> +++ b/gcc/tree-ssanames.h
>>>>> @@ -43,6 +43,9 @@ struct GTY(()) ptr_info_def
>>>>>       above alignment.  Access only through the same helper functions
>>>>> as
>>>>> align
>>>>>       above.  */
>>>>>    unsigned int misalign;
>>>>> +  /* When this pointer is knonw to be nnonnull this would be true
>>>>> otherwise
>>>>> +     false.  */
>>>>> +  bool  nonnull_p;
>>>>>  };
>>>>
>>>>
>>>>
>>>> Why do you need this?  Doesn't the pt.null bit represent that already?
>>>
>>>
>>>
>>> It looks like I can use this. As in gcc/tree-ssa-alias.h:
>>>
>>>   /* Nonzero if the points-to set includes 'nothing', the points-to set
>>>      includes memory at address NULL.  */
>>>   unsigned int null : 1;
>>>
>>> But in gcc/tree-ssa-alias.c, ptrs_compare_unequal has the following
>>> comment
>>> which says:
>>>
>>> /* ???  We'd like to handle ptr1 != NULL and ptr1 != ptr2
>>>      but those require pt.null to be conservatively correct.  */
>>>
>>> Does that means it can be wrong at times? I haven't looked it in detail
>>> yet
>>> but if it is, it would be a problem.
>>
>>
>> Yes, this bit is not correctly computed for all cases (well, it works
>> for trivial
>> ones but for example misses weaks and maybe some other corner-cases).
>>
>> Currently there are no consumers of this bit so the incorrectness
>> doesn't matter.
>>
>> I suggest you simply use that bit but set it conservatively from PTA (to
>> true)
>> for now and adjust it from VRP only.
>>
>> I do have some patches for fixinig some of the .nonnull_p issues in PTA
>> but
>> they come at a cost of more constraints.
>
>
> Here is a version of the patch that does this. I have a follow up patch to
> use this in ipa-vrp. I will send it for review once this is OK.
>
> Bootstrapped and regression tested on x86_64-linux-gnu with no new
> regressions. Is this OK for trunk?

@@ -6359,7 +6361,7 @@ find_what_var_points_to (tree fndecl, varinfo_t orig_vi)
       if (vi->is_artificial_var)
        {
          if (vi->id == nothing_id)
-           pt->null = 1;
+           ;

please leave this here.

 pt_solution_singleton_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;

humm... this is a correctness problem I guess.  A latent one currently
depending on who relies on it.
There is a single use of the above predicate which looks for the
points-to solution of a __builtin_alloca call.  We should somehow
arrange for its solution to not include ->null.  Or, simpler, change
the predicates name to pt_solution_singleton_or_null_p () as
the use does not care for pt->null.

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

 = false;

+}

There is the question on how pt.null semantics should be
with respect to pt.anything, pt.escaped and pt.nonlocal.
I think get_ptr_nonnull needs to return false if any
of those are set.

Richard.

>
> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2016-10-07  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         * tree-ssa-alias.c (dump_points_to_solution): Dump non null as it
>           is set to null conservatively.
>         * tree-ssa-structalias.c (find_what_var_points_to): Set to null
>           conservatively.
>         (pt_solution_singleton_p): Dont check null.
>         * 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.
>
>>
>> Richard.
>>
>>>> Also, formatting and spelling:
>>>> s/knonw/known/
>>>> s/nnon/non/
>>>> s/bool  /bool /
>>>
>>>
>>>
>>> I will change this.
>>>
>>> Thanks,
>>> Kugan
>>>>
>>>>
>>>>
>>>>         Jakub
>>>>
>>>
>
diff mbox

Patch

From 20ef028b2e38e3d91874ba873be10e5dfdac4d05 Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Date: Fri, 30 Sep 2016 09:00:08 +1000
Subject: [PATCH 3/5] Add nonnull to pointer from VRP

---
 gcc/tree-ssa-alias.c       |  4 ++--
 gcc/tree-ssa-structalias.c |  6 ++++--
 gcc/tree-ssanames.c        | 21 +++++++++++++++++++++
 gcc/tree-ssanames.h        |  2 ++
 gcc/tree-vrp.c             | 44 +++++++++++++++++++++++++++++---------------
 5 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
index 30de461..ccad68d 100644
--- a/gcc/tree-ssa-alias.c
+++ b/gcc/tree-ssa-alias.c
@@ -515,8 +515,8 @@  dump_points_to_solution (FILE *file, struct pt_solution *pt)
   if (pt->ipa_escaped)
     fprintf (file, ", points-to unit escaped");
 
-  if (pt->null)
-    fprintf (file, ", points-to NULL");
+  if (!pt->null)
+    fprintf (file, ", points-to non NULL");
 
   if (pt->vars)
     {
diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index be892fd..99b373f8 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -6349,6 +6349,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.  */
@@ -6359,7 +6361,7 @@  find_what_var_points_to (tree fndecl, varinfo_t orig_vi)
       if (vi->is_artificial_var)
 	{
 	  if (vi->id == nothing_id)
-	    pt->null = 1;
+	    ;
 	  else if (vi->id == escaped_id)
 	    {
 	      if (in_ipa_mode)
@@ -6569,7 +6571,7 @@  bool
 pt_solution_singleton_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 91a8f97..6b6619f 100644
--- a/gcc/tree-ssanames.c
+++ b/gcc/tree-ssanames.c
@@ -374,6 +374,27 @@  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;
+  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 0892709..80b7302 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -10593,18 +10593,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,
@@ -10808,17 +10814,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