diff mbox

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

Message ID 52A24F58.1070802@naturalbridge.com
State New
Headers show

Commit Message

Kenneth Zadeck Dec. 6, 2013, 10:27 p.m. UTC 
On 12/06/2013 01:32 PM, Richard Sandiford wrote:
> 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
>
so this one works the way you want.    While it is true the the problems 
are disjoint, the solution will likely evolving change the same lines of 
source in two different ways.

ok to commit.

kenny

Comments

Richard Sandiford Dec. 7, 2013, 10:40 a.m. UTC | #1 
Kenneth Zadeck <zadeck@naturalbridge.com> writes:
>>>>>    #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
>>
> so this one works the way you want.    While it is true the the problems 
> are disjoint, the solution will likely evolving change the same lines of 
> source in two different ways.

Well, if we're changing the definition of WIDE_INT_MAX_ELTS now (and we are)
I think we should change it to something that makes sense.  All we want is
1 extra bit.  We don't need to round up the size and then add a whole
HWI on top.  Doing that will potentially pessimise targets that don't
need anything beyond SImode.

It's not like I can approve the patch anyway though, so I'll leave it there.

Thanks,
Richard
Richard Biener Dec. 8, 2013, 10:35 a.m. UTC | #2 
Richard Sandiford <rdsandiford@googlemail.com> wrote:
>Kenneth Zadeck <zadeck@naturalbridge.com> writes:
>>>>>>    #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
>>>
>> so this one works the way you want.    While it is true the the
>problems 
>> are disjoint, the solution will likely evolving change the same lines
>of 
>> source in two different ways.
>
>Well, if we're changing the definition of WIDE_INT_MAX_ELTS now (and we
>are)
>I think we should change it to something that makes sense.  All we want
>is
>1 extra bit.  We don't need to round up the size and then add a whole
>HWI on top.  Doing that will potentially pessimise targets that don't
>need anything beyond SImode.

I agree.

Richard.

>It's not like I can approve the patch anyway though, so I'll leave it
>there.
>
>Thanks,
>Richard
diff mbox

Patch

Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c	(revision 205726)
+++ gcc/tree-vrp.c	(working copy)
@@ -2620,23 +2620,24 @@  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;
+	  typedef FIXED_WIDE_INT (WIDE_INT_MAX_PRECISION * 2) vrp_int;
+	  typedef generic_wide_int
+             <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> > vrp_int_cst;
+	  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 = 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);
 	  /* Canonicalize the intervals.  */
 	  if (sign == UNSIGNED)
 	    {
@@ -2653,17 +2654,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 +2672,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;