diff mbox

Set nonnull attribute to ptr_info_def based on VRP

Message ID 717fb095-f0a9-6980-ddbd-e755a4fd6457@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Aug. 8, 2016, 3:36 a.m. UTC 
Hi,

This patch tries to add nonnull_p attribute to ptr_info_def. As it 
stands, range_info_def is bigger in size than ptr_info_def. Therefore 
adding  nonnull_p should not change the size.

I am setting this based on the VRP analysis. Right there is no uses for 
this. But the idea is, this can be used in IPA-VRP such that redundant 
check for NULL can be eliminated.

Bootstrapped and regression tested on x86_64-linux-gnu. Is this OK for 
trunk.

Thanks,
Kugan


gcc/ChangeLog:

2016-08-08  Kugan Vivekanandarajah  <kuganv@linaro.org>

	* tree-ssanames.c (set_ptr_nonnull): New.
	(get_ptr_nonnull): Likewise.
	* tree-ssanames.h (struct ptr_info_def): Add nonnull_p attribute.
	* tree-vrp.c (vrp_finalize): Call set_ptr_nonnull.

Comments

Jakub Jelinek Aug. 8, 2016, 6:40 a.m. UTC | #1 
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?
Also, formatting and spelling:
s/knonw/known/
s/nnon/non/
s/bool  /bool /

	Jakub
Kugan Vivekanandarajah Aug. 8, 2016, 10:58 p.m. UTC | #2 
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.

> Also, formatting and spelling:
> s/knonw/known/
> s/nnon/non/
> s/bool  /bool /

I will change this.

Thanks,
Kugan
>
> 	Jakub
>
Richard Biener Aug. 9, 2016, 8:58 a.m. UTC | #3 
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.

Richard.

>> Also, formatting and spelling:
>> s/knonw/known/
>> s/nnon/non/
>> s/bool  /bool /
>
>
> I will change this.
>
> Thanks,
> Kugan
>>
>>
>>         Jakub
>>
>
diff mbox

Patch

diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c
index 91a8f97..19c9027 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, bool nonnull_p)
+{
+  gcc_assert (POINTER_TYPE_P (TREE_TYPE (name)));
+  struct ptr_info_def *pi = get_ptr_info (name);
+  pi->nonnull_p = nonnull_p;;
+}
+
+/* Return nonnull attribute of pointer NAME.  */
+bool
+get_ptr_nonnull (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->nonnull_p;
+}
+
 /* Change non-zero bits bitmask of NAME.  */
 
 void
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;
 };
 
 /* Value range information for SSA_NAMEs representing non-pointer variables.  */
@@ -89,6 +92,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 name, bool nonnull_p);
+extern bool get_ptr_nonnull (tree name);
 
 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 7c7ad91..40c4d48 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -10352,18 +10352,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, true);
+	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,