Re: Avoid generating useless range info

[Richard Biener]

On Wed, Jun 28, 2017 at 9:56 AM, Aldy Hernandez  wrote:
>
>
> On 06/27/2017 06:38 AM, Jakub Jelinek wrote:
>>
>> On Tue, Jun 27, 2017 at 06:26:46AM -0400, Aldy Hernandez wrote:
>>>
>>> How about this?
>>
>>
>> @@ -360,6 +363,22 @@ set_range_info (tree name, enum value_range_type
>> range_type,
>>   }
>>   }
>>   +/* Store range information RANGE_TYPE, MIN, and MAX to tree ssa_name
>> +   NAME while making sure we don't store useless range info.  */
>> +
>> +void
>> +set_range_info (tree name, enum value_range_type range_type,
>> +   const wide_int_ref , const wide_int_ref )
>> +{
>> +  /* A range of the entire domain is really no range at all.  */
>> +  tree type = TREE_TYPE (name);
>> +  if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
>> +  && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
>> +return;
>> +
>> +  set_range_info_raw (name, range_type, min, max);
>> +}
>> +
>>
>> Won't this misbehave if we have a narrower range on some SSA_NAME and
>> call set_range_info to make it VARYING?
>> In that case (i.e. SSA_NAME_RANGE_INFO (name) != NULL), we should either
>> set_range_info_raw too (if nonzero_bits is not all ones) or clear
>> SSA_NAME_RANGE_INFO (otherwise).
>
>
> Good point.  Fixed.
>
>> /* Gets range information MIN, MAX and returns enum value_range_type
>>  corresponding to tree ssa_name NAME.  enum value_range_type returned
>> @@ -419,9 +438,13 @@ set_nonzero_bits (tree name, const wide_int_ref
>> )
>>   {
>> gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
>> if (SSA_NAME_RANGE_INFO (name) == NULL)
>> -set_range_info (name, VR_RANGE,
>> -   TYPE_MIN_VALUE (TREE_TYPE (name)),
>> -   TYPE_MAX_VALUE (TREE_TYPE (name)));
>> +{
>> +  if (mask == -1)
>> +   return;
>> +  set_range_info_raw (name, VR_RANGE,
>> + TYPE_MIN_VALUE (TREE_TYPE (name)),
>> + TYPE_MAX_VALUE (TREE_TYPE (name)));
>> +}
>> range_info_def *ri = SSA_NAME_RANGE_INFO (name);
>> ri->set_nonzero_bits (mask);
>>
>> Similarly, if SSA_NAME_RANGE_INFO is previously non-NULL, but min/max
>> are VARYING and the new mask is -1, shouldn't we free it rather than
>> set it to the default?
>
>
> Here, if SSA_NAME_RANGE_INFO is previously non-NULL then we proceed as
> always-- just set the nonzero bits to whatever was specified (without
> clearning SSA_NAME_RANGE_INFO).  A mask of -1 and an SSA_NAME_RANGE_INFO of
> non-NULL can coexist just fine.
>
> How about this?

Ok.

Thanks,
Richard.

> Aldy

Re: Avoid generating useless range info

[Aldy Hernandez]

On 06/27/2017 06:38 AM, Jakub Jelinek wrote:

On Tue, Jun 27, 2017 at 06:26:46AM -0400, Aldy Hernandez wrote:

How about this?


@@ -360,6 +363,22 @@ set_range_info (tree name, enum value_range_type 
range_type,
  }
  }
  
+/* Store range information RANGE_TYPE, MIN, and MAX to tree ssa_name

+   NAME while making sure we don't store useless range info.  */
+
+void
+set_range_info (tree name, enum value_range_type range_type,
+   const wide_int_ref , const wide_int_ref )
+{
+  /* A range of the entire domain is really no range at all.  */
+  tree type = TREE_TYPE (name);
+  if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
+  && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
+return;
+
+  set_range_info_raw (name, range_type, min, max);
+}
+

Won't this misbehave if we have a narrower range on some SSA_NAME and
call set_range_info to make it VARYING?
In that case (i.e. SSA_NAME_RANGE_INFO (name) != NULL), we should either
set_range_info_raw too (if nonzero_bits is not all ones) or clear
SSA_NAME_RANGE_INFO (otherwise).


Good point.  Fixed.

  
  /* Gets range information MIN, MAX and returns enum value_range_type

 corresponding to tree ssa_name NAME.  enum value_range_type returned
@@ -419,9 +438,13 @@ set_nonzero_bits (tree name, const wide_int_ref )
  {
gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
if (SSA_NAME_RANGE_INFO (name) == NULL)
-set_range_info (name, VR_RANGE,
-   TYPE_MIN_VALUE (TREE_TYPE (name)),
-   TYPE_MAX_VALUE (TREE_TYPE (name)));
+{
+  if (mask == -1)
+   return;
+  set_range_info_raw (name, VR_RANGE,
+ TYPE_MIN_VALUE (TREE_TYPE (name)),
+ TYPE_MAX_VALUE (TREE_TYPE (name)));
+}
range_info_def *ri = SSA_NAME_RANGE_INFO (name);
ri->set_nonzero_bits (mask);

Similarly, if SSA_NAME_RANGE_INFO is previously non-NULL, but min/max
are VARYING and the new mask is -1, shouldn't we free it rather than
set it to the default?


Here, if SSA_NAME_RANGE_INFO is previously non-NULL then we proceed as 
always-- just set the nonzero bits to whatever was specified (without 
clearning SSA_NAME_RANGE_INFO).  A mask of -1 and an SSA_NAME_RANGE_INFO 
of non-NULL can coexist just fine.


How about this?

Aldy
gcc/

* tree-ssanames.c (set_range_info_raw): Abstract from ...
(set_range_info): ...here.  Only call set_range_info_raw if domain
is useful.
(set_nonzero_bits): Call set_range_info_raw.
* tree-ssanames.h (set_range_info_raw): New.

gcc/testsuite/

* gcc.dg/Walloca-14.c: Adapt test to recognize new complaint of
unbounded use.

diff --git a/gcc/testsuite/gcc.dg/Walloca-14.c 
b/gcc/testsuite/gcc.dg/Walloca-14.c
index 723dbe5..f3e3f57 100644
--- a/gcc/testsuite/gcc.dg/Walloca-14.c
+++ b/gcc/testsuite/gcc.dg/Walloca-14.c
@@ -9,5 +9,6 @@ g (int *p)
   extern void f (void *);
 
   void *q = __builtin_alloca (p); /* { dg-warning "passing argument 1" } */
+  /* { dg-warning "unbounded use of 'alloca'" "unbounded" { target *-*-* } 11 
} */
   f (q);
 }
diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c
index 353c7b1..0053b01 100644
--- a/gcc/tree-ssanames.c
+++ b/gcc/tree-ssanames.c
@@ -320,11 +320,14 @@ make_ssa_name_fn (struct function *fn, tree var, gimple 
*stmt,
   return t;
 }
 
-/* Store range information RANGE_TYPE, MIN, and MAX to tree ssa_name NAME.  */
+/* Helper function for set_range_info.
+
+   Store range information RANGE_TYPE, MIN, and MAX to tree ssa_name
+   NAME.  */
 
 void
-set_range_info (tree name, enum value_range_type range_type,
-   const wide_int_ref , const wide_int_ref )
+set_range_info_raw (tree name, enum value_range_type range_type,
+   const wide_int_ref , const wide_int_ref )
 {
   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
   gcc_assert (range_type == VR_RANGE || range_type == VR_ANTI_RANGE);
@@ -360,6 +363,34 @@ set_range_info (tree name, enum value_range_type 
range_type,
 }
 }
 
+/* Store range information RANGE_TYPE, MIN, and MAX to tree ssa_name
+   NAME while making sure we don't store useless range info.  */
+
+void
+set_range_info (tree name, enum value_range_type range_type,
+   const wide_int_ref , const wide_int_ref )
+{
+  gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
+
+  /* A range of the entire domain is really no range at all.  */
+  tree type = TREE_TYPE (name);
+  if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
+  && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
+{
+  range_info_def *ri = SSA_NAME_RANGE_INFO (name);
+  if (ri == NULL)
+   return;
+  if (ri->get_nonzero_bits () == -1)
+   {
+ ggc_free (ri);
+ SSA_NAME_RANGE_INFO (name) = NULL;
+ return;
+   }
+}
+
+  set_range_info_raw (name, range_type, min, max);
+}
+
 
 /* Gets range information MIN, MAX and 

Re: Avoid generating useless range info

[Jakub Jelinek]

On Tue, Jun 27, 2017 at 06:26:46AM -0400, Aldy Hernandez wrote:
> How about this?

@@ -360,6 +363,22 @@ set_range_info (tree name, enum value_range_type 
range_type,
 }
 }
 
+/* Store range information RANGE_TYPE, MIN, and MAX to tree ssa_name
+   NAME while making sure we don't store useless range info.  */
+
+void
+set_range_info (tree name, enum value_range_type range_type,
+   const wide_int_ref , const wide_int_ref )
+{
+  /* A range of the entire domain is really no range at all.  */
+  tree type = TREE_TYPE (name);
+  if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
+  && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
+return;
+
+  set_range_info_raw (name, range_type, min, max);
+}
+

Won't this misbehave if we have a narrower range on some SSA_NAME and
call set_range_info to make it VARYING?
In that case (i.e. SSA_NAME_RANGE_INFO (name) != NULL), we should either
set_range_info_raw too (if nonzero_bits is not all ones) or clear
SSA_NAME_RANGE_INFO (otherwise).
 
 /* Gets range information MIN, MAX and returns enum value_range_type
corresponding to tree ssa_name NAME.  enum value_range_type returned
@@ -419,9 +438,13 @@ set_nonzero_bits (tree name, const wide_int_ref )
 {
   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
   if (SSA_NAME_RANGE_INFO (name) == NULL)
-set_range_info (name, VR_RANGE,
-   TYPE_MIN_VALUE (TREE_TYPE (name)),
-   TYPE_MAX_VALUE (TREE_TYPE (name)));
+{
+  if (mask == -1)
+   return;
+  set_range_info_raw (name, VR_RANGE,
+ TYPE_MIN_VALUE (TREE_TYPE (name)),
+ TYPE_MAX_VALUE (TREE_TYPE (name)));
+}
   range_info_def *ri = SSA_NAME_RANGE_INFO (name);
   ri->set_nonzero_bits (mask);

Similarly, if SSA_NAME_RANGE_INFO is previously non-NULL, but min/max
are VARYING and the new mask is -1, shouldn't we free it rather than
set it to the default?

If we consider the cases rare enough to worry about, at least your
above
  if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
  && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
should be
  gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
  if (SSA_NAME_RANGE_INFO (name) == NULL
  && min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
  && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
We'd then not misbehave, just might in some rare cases keep
SSA_NAME_RANGE_INFO non-NULL even if it contains the default stuff.

Jakub

Re: Avoid generating useless range info

[Aldy Hernandez]

On Fri, Jun 23, 2017 at 6:24 AM, Richard Biener
 wrote:
> On Fri, Jun 23, 2017 at 10:59 AM, Aldy Hernandez  wrote:
>>
>>
>> On Fri, Jun 16, 2017 at 4:00 AM, Richard Biener 
>> wrote:
>>>
>>> On Wed, Jun 14, 2017 at 6:41 PM, Aldy Hernandez  wrote:
>>> > Hi!
>>> >
>>> > As discovered in my range class work, we seem to generate a significant
>>> > amount of useless range info out of VRP.
>>> >
>>> > Is there any reason why we can't avoid generating any range info that
>>> > spans
>>> > the entire domain, and yet contains nothing in the non-zero bitmask?
>>> >
>>> > The attached patch passes bootstrap, and the one regression it causes is
>>> > because now the -Walloca-larger-than= pass is better able to determine
>>> > that
>>> > there is no range information at all, and the testcase is unbounded.
>>> > So...win, win.
>>> >
>>> > OK for trunk?
>>>
>>> Can you please do this in set_range_info itself?  Thus, if min ==
>>> wi::min_value && max == wi::max_value
>>> simply return?  (do not use TYPE_MIN?MAX_VALUE please)
>>
>>
>> The reason I did it in vrp_finalize is because if you do it in
>> set_range_info, you break set_nonzero_bits when setting bits on an SSA that
>> currently has no range info:
>>
>> void
>> set_nonzero_bits (tree name, const wide_int_ref )
>> {
>>   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
>>   if (SSA_NAME_RANGE_INFO (name) == NULL)
>> set_range_info (name, VR_RANGE,
>>TYPE_MIN_VALUE (TREE_TYPE (name)),
>>TYPE_MAX_VALUE (TREE_TYPE (name)));
>>   range_info_def *ri = SSA_NAME_RANGE_INFO (name);
>>   ri->set_nonzero_bits (mask);
>> }
>>
>> Let me know how you'd like me to proceed.
>
> Just factor out a set_range_info_raw and call that then from here.

How about this?

Aldy


curr
Description: Binary data

Re: Avoid generating useless range info

[Richard Biener]

On Fri, Jun 23, 2017 at 12:32 PM, Jakub Jelinek  wrote:
> On Fri, Jun 23, 2017 at 12:24:25PM +0200, Richard Biener wrote:
>> > void
>> > set_nonzero_bits (tree name, const wide_int_ref )
>> > {
>> >   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
>> >   if (SSA_NAME_RANGE_INFO (name) == NULL)
>> > set_range_info (name, VR_RANGE,
>> >TYPE_MIN_VALUE (TREE_TYPE (name)),
>> >TYPE_MAX_VALUE (TREE_TYPE (name)));
>> >   range_info_def *ri = SSA_NAME_RANGE_INFO (name);
>> >   ri->set_nonzero_bits (mask);
>> > }
>> >
>> > Let me know how you'd like me to proceed.
>>
>> Just factor out a set_range_info_raw and call that then from here.
>
> And don't call it if the mask is all ones.  Perhaps set_range_info
> and set_nonzero_bits even should ggc_free and clear earlier range_info_def
> if the range is all values and nonzero bit mask is all ones.
> Or do we share range_info_def between multiple SSA_NAMEs?  If yes, of course
> we shouldn't use ggc_free.

We shouldn't as we don't copy on change.  We do for points-to but only for the
bitmap pointer IIRC.

Richard,

>
> Jakub

Re: Avoid generating useless range info

[Jakub Jelinek]

On Fri, Jun 23, 2017 at 12:24:25PM +0200, Richard Biener wrote:
> > void
> > set_nonzero_bits (tree name, const wide_int_ref )
> > {
> >   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
> >   if (SSA_NAME_RANGE_INFO (name) == NULL)
> > set_range_info (name, VR_RANGE,
> >TYPE_MIN_VALUE (TREE_TYPE (name)),
> >TYPE_MAX_VALUE (TREE_TYPE (name)));
> >   range_info_def *ri = SSA_NAME_RANGE_INFO (name);
> >   ri->set_nonzero_bits (mask);
> > }
> >
> > Let me know how you'd like me to proceed.
> 
> Just factor out a set_range_info_raw and call that then from here.

And don't call it if the mask is all ones.  Perhaps set_range_info
and set_nonzero_bits even should ggc_free and clear earlier range_info_def
if the range is all values and nonzero bit mask is all ones.
Or do we share range_info_def between multiple SSA_NAMEs?  If yes, of course
we shouldn't use ggc_free.

Jakub

Re: Avoid generating useless range info

[Richard Biener]

On Fri, Jun 23, 2017 at 10:59 AM, Aldy Hernandez  wrote:
>
>
> On Fri, Jun 16, 2017 at 4:00 AM, Richard Biener 
> wrote:
>>
>> On Wed, Jun 14, 2017 at 6:41 PM, Aldy Hernandez  wrote:
>> > Hi!
>> >
>> > As discovered in my range class work, we seem to generate a significant
>> > amount of useless range info out of VRP.
>> >
>> > Is there any reason why we can't avoid generating any range info that
>> > spans
>> > the entire domain, and yet contains nothing in the non-zero bitmask?
>> >
>> > The attached patch passes bootstrap, and the one regression it causes is
>> > because now the -Walloca-larger-than= pass is better able to determine
>> > that
>> > there is no range information at all, and the testcase is unbounded.
>> > So...win, win.
>> >
>> > OK for trunk?
>>
>> Can you please do this in set_range_info itself?  Thus, if min ==
>> wi::min_value && max == wi::max_value
>> simply return?  (do not use TYPE_MIN?MAX_VALUE please)
>
>
> The reason I did it in vrp_finalize is because if you do it in
> set_range_info, you break set_nonzero_bits when setting bits on an SSA that
> currently has no range info:
>
> void
> set_nonzero_bits (tree name, const wide_int_ref )
> {
>   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
>   if (SSA_NAME_RANGE_INFO (name) == NULL)
> set_range_info (name, VR_RANGE,
>TYPE_MIN_VALUE (TREE_TYPE (name)),
>TYPE_MAX_VALUE (TREE_TYPE (name)));
>   range_info_def *ri = SSA_NAME_RANGE_INFO (name);
>   ri->set_nonzero_bits (mask);
> }
>
> Let me know how you'd like me to proceed.

Just factor out a set_range_info_raw and call that then from here.

Richard.

> Aldy
>
>>
>> Thanks,
>> Richard.
>>
>> > Aldy
>
>

Re: Avoid generating useless range info

[Aldy Hernandez]

[one more time, but without sending html which the list refuses :-/]

On Fri, Jun 16, 2017 at 4:00 AM, Richard Biener
 wrote:
> On Wed, Jun 14, 2017 at 6:41 PM, Aldy Hernandez  wrote:
>> Hi!
>>
>> As discovered in my range class work, we seem to generate a significant
>> amount of useless range info out of VRP.
>>
>> Is there any reason why we can't avoid generating any range info that spans
>> the entire domain, and yet contains nothing in the non-zero bitmask?
>>
>> The attached patch passes bootstrap, and the one regression it causes is
>> because now the -Walloca-larger-than= pass is better able to determine that
>> there is no range information at all, and the testcase is unbounded.
>> So...win, win.
>>
>> OK for trunk?
>
> Can you please do this in set_range_info itself?  Thus, if min ==
> wi::min_value && max == wi::max_value
> simply return?  (do not use TYPE_MIN?MAX_VALUE please)


The reason I did it in vrp_finalize is because if you do it in
set_range_info, you break set_nonzero_bits when setting bits on an SSA
that currently has no range info:

void
set_nonzero_bits (tree name, const wide_int_ref )
{
  gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
  if (SSA_NAME_RANGE_INFO (name) == NULL)
set_range_info (name, VR_RANGE,
   TYPE_MIN_VALUE (TREE_TYPE (name)),
   TYPE_MAX_VALUE (TREE_TYPE (name)));
  range_info_def *ri = SSA_NAME_RANGE_INFO (name);
  ri->set_nonzero_bits (mask);
}

Let me know how you'd like me to proceed.
Aldy

>
> Thanks,
> Richard.
>
>> Aldy

Re: Avoid generating useless range info

[Richard Biener]

On Wed, Jun 14, 2017 at 6:41 PM, Aldy Hernandez  wrote:
> Hi!
>
> As discovered in my range class work, we seem to generate a significant
> amount of useless range info out of VRP.
>
> Is there any reason why we can't avoid generating any range info that spans
> the entire domain, and yet contains nothing in the non-zero bitmask?
>
> The attached patch passes bootstrap, and the one regression it causes is
> because now the -Walloca-larger-than= pass is better able to determine that
> there is no range information at all, and the testcase is unbounded.
> So...win, win.
>
> OK for trunk?

Can you please do this in set_range_info itself?  Thus, if min ==
wi::min_value && max == wi::max_value
simply return?  (do not use TYPE_MIN?MAX_VALUE please)

Thanks,
Richard.

> Aldy