diff mbox

[wide-int] small cleanup in wide-int.*

Message ID 52A1FF29.3050909@naturalbridge.com
State New
Headers show

Commit Message

Kenneth Zadeck Dec. 6, 2013, 4:45 p.m. UTC 
On 12/03/2013 11:52 AM, Richard Sandiford wrote:
> Kenneth Zadeck <zadeck@naturalbridge.com> writes:
>> Index: tree-vrp.c
>> ===================================================================
>> --- tree-vrp.c	(revision 205597)
>> +++ tree-vrp.c	(working copy)
>> @@ -2611,22 +2611,28 @@ extract_range_from_binary_expr_1 (value_
>>   
>>         signop sign = TYPE_SIGN (expr_type);
>>         unsigned int prec = TYPE_PRECISION (expr_type);
>> -      unsigned int prec2 = (prec * 2) + (sign == UNSIGNED ? 2 : 0);
>>   
>>         if (range_int_cst_p (&vr0)
>>   	  && range_int_cst_p (&vr1)
>>   	  && TYPE_OVERFLOW_WRAPS (expr_type))
>>   	{
>> -	  wide_int sizem1 = wi::mask (prec, false, prec2);
>> -	  wide_int size = sizem1 + 1;
>> +	  /* vrp_int is twice as wide as anything that the target
>> +	     supports so it can support a full width multiply.  No
>> +	     need to add any more padding for an extra sign bit
>> +	     because that comes with the way that WIDE_INT_MAX_ELTS is
>> +	     defined.  */
>> +	  typedef FIXED_WIDE_INT (WIDE_INT_MAX_PRECISION * 2)
>> +	    vrp_int;
>> +	  vrp_int sizem1 = wi::mask <vrp_int> (prec, false);
>> +	  vrp_int size = sizem1 + 1;
>>   
>>   	  /* Extend the values using the sign of the result to PREC2.
>>   	     From here on out, everthing is just signed math no matter
>>   	     what the input types were.  */
>> -	  wide_int min0 = wide_int::from (vr0.min, prec2, sign);
>> -	  wide_int max0 = wide_int::from (vr0.max, prec2, sign);
>> -	  wide_int min1 = wide_int::from (vr1.min, prec2, sign);
>> -	  wide_int max1 = wide_int::from (vr1.max, prec2, sign);
>> +	  vrp_int min0 = wi::to_vrp (vr0.min);
>> +	  vrp_int max0 = wi::to_vrp (vr0.max);
>> +	  vrp_int min1 = wi::to_vrp (vr1.min);
>> +	  vrp_int max1 = wi::to_vrp (vr1.max);
> I think we should avoid putting to_vrp in tree.h if vrp_int is only
> local to this block.  Instead you could have:
>
> 	  typedef generic_wide_int
>               <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> > vrp_int_cst;
>            ...
>            vrp_int_cst min0 = vr0.min;
>            vrp_int_cst max0 = vr0.max;
>            vrp_int_cst min1 = vr1.min;
>            vrp_int_cst max1 = vr1.max;
>
i did this in a different way because i had trouble doing it as you 
suggested.    the short answer is that all of the vrp_int code is now 
local to tree-vrp.c which i think was your primary goal
>> @@ -228,15 +228,16 @@ along with GCC; see the file COPYING3.
>>   #endif
>>   
>>   /* The MAX_BITSIZE_MODE_ANY_INT is automatically generated by a very
>> -   early examination of the target's mode file.  Thus it is safe that
>> -   some small multiple of this number is easily larger than any number
>> -   that that target could compute.  The place in the compiler that
>> -   currently needs the widest ints is the code that determines the
>> -   range of a multiply.  This code needs 2n + 2 bits.  */
>> -
>> +   early examination of the target's mode file.  The WIDE_INT_MAX_ELTS
>> +   can accomodate at least 1 more bit so that unsigned numbers of that
>> +   mode can be represented.  This will accomodate every place in the
>> +   compiler except for a multiply routine in tree-vrp.  That function
>> +   makes its own arrangements for larger wide-ints.  */
> I think we should drop the "This will accomodate..." bit, since it'll soon
> get out of date.  Maybe something like:
>
>      Note that it is still possible to create fixed_wide_ints that have
>      precisions greater than MAX_BITSIZE_MODE_ANY_INT.  This can be useful
>      when representing a double-width multiplication result, for example.  */
done
>>   #define WIDE_INT_MAX_ELTS \
>> -  ((4 * MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT - 1) \
>> -   / HOST_BITS_PER_WIDE_INT)
>> +  (((MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT - 1)	\
>> +    / HOST_BITS_PER_WIDE_INT) + 1)
> I think this should be:
>
>    (MAX_BITSIZE_MODE_ANY_INT / HOST_BITS_PER_WIDE_INT + 1)
>
> We only need an extra HWI if MAX_BITSIZE_MODE_ANY_INT is an exact multiple
> of HOST_BITS_PER_WIDE_INT.
we will do this later when some other issues that Eric B raised are settled.

ok to commit to the branch?

kenny
> Looks good to me otherwise FWIW.
>
> You probably already realise this, but for avoidance of doubt, Richard
> was also asking that we reduce MAX_BITSIZE_MODE_ANY_INT to 128 on x86_64,
> since that's the largest scalar_mode_supported_p mode.
>
> Thanks,
> Richard
>

Comments

Richard Sandiford Dec. 6, 2013, 6:32 p.m. UTC | #1 
Kenneth Zadeck <zadeck@naturalbridge.com> writes:
> On 12/03/2013 11:52 AM, Richard Sandiford wrote:
>> Kenneth Zadeck <zadeck@naturalbridge.com> writes:
>>> Index: tree-vrp.c
>>> ===================================================================
>>> --- tree-vrp.c	(revision 205597)
>>> +++ tree-vrp.c	(working copy)
>>> @@ -2611,22 +2611,28 @@ extract_range_from_binary_expr_1 (value_
>>>   
>>>         signop sign = TYPE_SIGN (expr_type);
>>>         unsigned int prec = TYPE_PRECISION (expr_type);
>>> -      unsigned int prec2 = (prec * 2) + (sign == UNSIGNED ? 2 : 0);
>>>   
>>>         if (range_int_cst_p (&vr0)
>>>   	  && range_int_cst_p (&vr1)
>>>   	  && TYPE_OVERFLOW_WRAPS (expr_type))
>>>   	{
>>> -	  wide_int sizem1 = wi::mask (prec, false, prec2);
>>> -	  wide_int size = sizem1 + 1;
>>> +	  /* vrp_int is twice as wide as anything that the target
>>> +	     supports so it can support a full width multiply.  No
>>> +	     need to add any more padding for an extra sign bit
>>> +	     because that comes with the way that WIDE_INT_MAX_ELTS is
>>> +	     defined.  */
>>> +	  typedef FIXED_WIDE_INT (WIDE_INT_MAX_PRECISION * 2)
>>> +	    vrp_int;
>>> +	  vrp_int sizem1 = wi::mask <vrp_int> (prec, false);
>>> +	  vrp_int size = sizem1 + 1;
>>>   
>>>   	  /* Extend the values using the sign of the result to PREC2.
>>>   	     From here on out, everthing is just signed math no matter
>>>   	     what the input types were.  */
>>> -	  wide_int min0 = wide_int::from (vr0.min, prec2, sign);
>>> -	  wide_int max0 = wide_int::from (vr0.max, prec2, sign);
>>> -	  wide_int min1 = wide_int::from (vr1.min, prec2, sign);
>>> -	  wide_int max1 = wide_int::from (vr1.max, prec2, sign);
>>> +	  vrp_int min0 = wi::to_vrp (vr0.min);
>>> +	  vrp_int max0 = wi::to_vrp (vr0.max);
>>> +	  vrp_int min1 = wi::to_vrp (vr1.min);
>>> +	  vrp_int max1 = wi::to_vrp (vr1.max);
>> I think we should avoid putting to_vrp in tree.h if vrp_int is only
>> local to this block.  Instead you could have:
>>
>> 	  typedef generic_wide_int
>>               <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> > vrp_int_cst;
>>            ...
>>            vrp_int_cst min0 = vr0.min;
>>            vrp_int_cst max0 = vr0.max;
>>            vrp_int_cst min1 = vr1.min;
>>            vrp_int_cst max1 = vr1.max;
>>
> i did this in a different way because i had trouble doing it as you 
> suggested.    the short answer is that all of the vrp_int code is now 
> local to tree-vrp.c which i think was your primary goal

Ah, so we later assign to these variables:

	  /* Canonicalize the intervals.  */
	  if (sign == UNSIGNED)
	    {
	      if (wi::ltu_p (size, min0 + max0))
		{
		  min0 -= size;
		  max0 -= size;
		}

	      if (wi::ltu_p (size, min1 + max1))
		{
		  min1 -= size;
		  max1 -= size;
		}
	    }

OK, in that case I suppose a temporary is needed.  But I'd prefer
not to put local stuff in the wi:: namespace.  You could just have:

	  typedef generic_wide_int
	    <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> > vrp_int_cst;

          vrp_int min0 = vrp_int_cst (vr0.min);
          vrp_int max0 = vrp_int_cst (vr0.max);
          vrp_int min1 = vrp_int_cst (vr1.min);
          vrp_int max1 = vrp_int_cst (vr1.max);

which removes the need for:

+/* vrp_int is twice as wide as anything that the target supports so it
+   can support a full width multiply.  No need to add any more padding
+   for an extra sign bit because that comes with the way that
+   WIDE_INT_MAX_ELTS is defined.  */ 
+typedef FIXED_WIDE_INT (WIDE_INT_MAX_PRECISION * 2) vrp_int;
+namespace wi
+{
+  generic_wide_int <extended_tree <WIDE_INT_MAX_PRECISION * 2> > to_vrp (const_tree);
+}
+
+inline generic_wide_int <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> >
+wi::to_vrp (const_tree t)
+{
+  return t;
+}
+

>>>   #define WIDE_INT_MAX_ELTS \
>>> -  ((4 * MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT - 1) \
>>> -   / HOST_BITS_PER_WIDE_INT)
>>> +  (((MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT - 1)	\
>>> +    / HOST_BITS_PER_WIDE_INT) + 1)
>> I think this should be:
>>
>>    (MAX_BITSIZE_MODE_ANY_INT / HOST_BITS_PER_WIDE_INT + 1)
>>
>> We only need an extra HWI if MAX_BITSIZE_MODE_ANY_INT is an exact multiple
>> of HOST_BITS_PER_WIDE_INT.
> we will do this later when some other issues that Eric B raised are settled.

I think you're talking about defining MAX_BITSIZE_MODE_ANY_INT as
MAX_SIZE_MODE_ANY_INT * BITS_PER_UNIT, but that's orthogonal to the
change above.  IMO it doesn't make sense to both round up the division
and also add 1 to the result.  So I think we should make this change
regardless of whatever follows.

Looks good to me otherwise, thanks.

Richard
diff mbox

Patch

Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c	(revision 205726)
+++ gcc/tree-vrp.c	(working copy)
@@ -2213,6 +2213,22 @@  extract_range_from_multiplicative_op_1 (
     set_value_range (vr, type, min, max, NULL);
 }
 
+/* vrp_int is twice as wide as anything that the target supports so it
+   can support a full width multiply.  No need to add any more padding
+   for an extra sign bit because that comes with the way that
+   WIDE_INT_MAX_ELTS is defined.  */ 
+typedef FIXED_WIDE_INT (WIDE_INT_MAX_PRECISION * 2) vrp_int;
+namespace wi
+{
+  generic_wide_int <extended_tree <WIDE_INT_MAX_PRECISION * 2> > to_vrp (const_tree);
+}
+
+inline generic_wide_int <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> >
+wi::to_vrp (const_tree t)
+{
+  return t;
+}
+
 /* Extract range information from a binary operation CODE based on
    the ranges of each of its operands, *VR0 and *VR1 with resulting
    type EXPR_TYPE.  The resulting range is stored in *VR.  */
@@ -2620,22 +2636,21 @@  extract_range_from_binary_expr_1 (value_
 
       signop sign = TYPE_SIGN (expr_type);
       unsigned int prec = TYPE_PRECISION (expr_type);
-      unsigned int prec2 = (prec * 2) + (sign == UNSIGNED ? 2 : 0);
 
       if (range_int_cst_p (&vr0)
 	  && range_int_cst_p (&vr1)
 	  && TYPE_OVERFLOW_WRAPS (expr_type))
 	{
-	  wide_int sizem1 = wi::mask (prec, false, prec2);
-	  wide_int size = sizem1 + 1;
+	  vrp_int sizem1 = wi::mask <vrp_int> (prec, false);
+	  vrp_int size = sizem1 + 1;
 
 	  /* Extend the values using the sign of the result to PREC2.
 	     From here on out, everthing is just signed math no matter
 	     what the input types were.  */
-	  wide_int min0 = wide_int::from (vr0.min, prec2, sign);
-	  wide_int max0 = wide_int::from (vr0.max, prec2, sign);
-	  wide_int min1 = wide_int::from (vr1.min, prec2, sign);
-	  wide_int max1 = wide_int::from (vr1.max, prec2, sign);
+	  vrp_int min0 = wi::to_vrp (vr0.min);
+	  vrp_int max0 = wi::to_vrp (vr0.max);
+	  vrp_int min1 = wi::to_vrp (vr1.min);
+	  vrp_int max1 = wi::to_vrp (vr1.max);
 
 	  /* Canonicalize the intervals.  */
 	  if (sign == UNSIGNED)
@@ -2653,17 +2668,17 @@  extract_range_from_binary_expr_1 (value_
 		}
 	    }
 
-	  wide_int prod0 = min0 * min1;
-	  wide_int prod1 = min0 * max1;
-	  wide_int prod2 = max0 * min1;
-	  wide_int prod3 = max0 * max1;
+	  vrp_int prod0 = min0 * min1;
+	  vrp_int prod1 = min0 * max1;
+	  vrp_int prod2 = max0 * min1;
+	  vrp_int prod3 = max0 * max1;
 
 	  /* Sort the 4 products so that min is in prod0 and max is in
 	     prod3.  */
 	  /* min0min1 > max0max1 */
 	  if (wi::gts_p (prod0, prod3))
 	    {
-	      wide_int tmp = prod3;
+	      vrp_int tmp = prod3;
 	      prod3 = prod0;
 	      prod0 = tmp;
 	    }
@@ -2671,21 +2686,21 @@  extract_range_from_binary_expr_1 (value_
 	  /* min0max1 > max0min1 */
 	  if (wi::gts_p (prod1, prod2))
 	    {
-	      wide_int tmp = prod2;
+	      vrp_int tmp = prod2;
 	      prod2 = prod1;
 	      prod1 = tmp;
 	    }
 
 	  if (wi::gts_p (prod0, prod1))
 	    {
-	      wide_int tmp = prod1;
+	      vrp_int tmp = prod1;
 	      prod1 = prod0;
 	      prod0 = tmp;
 	    }
 
 	  if (wi::gts_p (prod2, prod3))
 	    {
-	      wide_int tmp = prod3;
+	      vrp_int tmp = prod3;
 	      prod3 = prod2;
 	      prod2 = tmp;
 	    }
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 205726)
+++ gcc/tree.h	(working copy)
@@ -4549,7 +4549,7 @@  namespace wi
     static const unsigned int precision = N;
   };
 
-  generic_wide_int <extended_tree <MAX_BITSIZE_MODE_ANY_INT> >
+  generic_wide_int <extended_tree <WIDE_INT_MAX_PRECISION> >
   to_widest (const_tree);
 
   generic_wide_int <extended_tree <ADDR_MAX_PRECISION> > to_offset (const_tree);
@@ -4570,7 +4570,7 @@  wi::int_traits <const_tree>::decompose (
 			  precision);
 }
 
-inline generic_wide_int <wi::extended_tree <MAX_BITSIZE_MODE_ANY_INT> >
+inline generic_wide_int <wi::extended_tree <WIDE_INT_MAX_PRECISION> >
 wi::to_widest (const_tree t)
 {
   return t;
@@ -4609,7 +4609,7 @@  wi::extended_tree <N>::get_len () const
 {
   if (N == ADDR_MAX_PRECISION)
     return TREE_INT_CST_OFFSET_NUNITS (m_t);
-  else if (N == MAX_BITSIZE_MODE_ANY_INT)
+  else if (N >= WIDE_INT_MAX_PRECISION)
     return TREE_INT_CST_EXT_NUNITS (m_t);
   else
     /* This class is designed to be used for specific output precisions
Index: gcc/wide-int.h
===================================================================
--- gcc/wide-int.h	(revision 205726)
+++ gcc/wide-int.h	(working copy)
@@ -228,15 +228,17 @@  along with GCC; see the file COPYING3.
 #endif
 
 /* The MAX_BITSIZE_MODE_ANY_INT is automatically generated by a very
-   early examination of the target's mode file.  Thus it is safe that
-   some small multiple of this number is easily larger than any number
-   that that target could compute.  The place in the compiler that
-   currently needs the widest ints is the code that determines the
-   range of a multiply.  This code needs 2n + 2 bits.  */
-
+   early examination of the target's mode file.  The WIDE_INT_MAX_ELTS
+   can accomodate at least 1 more bit so that unsigned numbers of that
+   mode can be represented.  Note that it is still possible to create
+   fixed_wide_ints that have precisions greater than
+   MAX_BITSIZE_MODE_ANY_INT.  This can be useful when representing a
+   double-width multiplication result, for example.  */
 #define WIDE_INT_MAX_ELTS \
-  ((4 * MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT - 1) \
-   / HOST_BITS_PER_WIDE_INT)
+  (((MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT - 1)	\
+    / HOST_BITS_PER_WIDE_INT) + 1)
+
+#define WIDE_INT_MAX_PRECISION (WIDE_INT_MAX_ELTS * HOST_BITS_PER_WIDE_INT)
 
 /* This is the max size of any pointer on any machine.  It does not
    seem to be as easy to sniff this out of the machine description as
@@ -294,7 +296,7 @@  struct wide_int_storage;
 
 typedef generic_wide_int <wide_int_storage> wide_int;
 typedef FIXED_WIDE_INT (ADDR_MAX_PRECISION) offset_int;
-typedef FIXED_WIDE_INT (MAX_BITSIZE_MODE_ANY_INT) widest_int;
+typedef FIXED_WIDE_INT (WIDE_INT_MAX_PRECISION) widest_int;
 
 template <bool SE>
 struct wide_int_ref_storage;