Patchwork [4/6] Fix computation of precision.

login
register
mail settings
Submitter Sebastian Pop
Date June 29, 2011, 5:35 p.m.
Message ID <1309368922-25238-5-git-send-email-sebpop@gmail.com>
Download mbox | patch
Permalink /patch/102647/
State New
Headers show

Comments

Sebastian Pop - June 29, 2011, 5:35 p.m.
2011-06-29  Sebastian Pop  <sebastian.pop@amd.com>

	* graphite-clast-to-gimple.c (precision_for_value): Removed.
	(precision_for_interval): Removed.
	(gcc_type_for_interval): Use mpz_sizeinbase.
---
 gcc/ChangeLog                  |    6 +++
 gcc/graphite-clast-to-gimple.c |   77 +++++-----------------------------------
 2 files changed, 15 insertions(+), 68 deletions(-)
Tobias Grosser - June 29, 2011, 11:16 p.m.
On 06/29/2011 12:35 PM, Sebastian Pop wrote:
> 2011-06-29  Sebastian Pop<sebastian.pop@amd.com>
>
> 	* graphite-clast-to-gimple.c (precision_for_value): Removed.
> 	(precision_for_interval): Removed.
> 	(gcc_type_for_interval): Use mpz_sizeinbase.
> -/* Return a type that could represent the integer value VAL.  */
> +/* Return a type that could represent the values between LOW and UP.
> +   The value of LOW can be bigger than UP.  */
>
>   static tree
>   gcc_type_for_interval (mpz_t low, mpz_t up)
>   {

Hi Sebastian,

why do we continue to call low 'low' and up 'up', if we actually just 
have two values v1 and v2 where we do not know which one is larger? I 
think this wrong and probably comes because we pass the lower loop bound 
to val_one and the upper loop bound to val_two.

What about:

+/* Return a type that could represent all values between VAL_ONE and
+   VAL_TWO including VAL_ONE and VAL_TWO itself.  There is no
+   constraint on which of the two values is larger.  */

   static tree
- gcc_type_for_interval (mpz_t low, mpz_t up)
+ gcc_type_for_interval (mpz_t val_one, mpz_t val_two)
    {

> -  bool unsigned_p = true;
> -  int precision, prec_up, prec_int;
> +  bool unsigned_p;
>     tree type;
>     enum machine_mode mode;
> -
> -  gcc_assert (mpz_cmp (low, up)<= 0);
> -
> -  prec_up = precision_for_value (up);
> -  prec_int = precision_for_interval (low, up);
> -  precision = MAX (prec_up, prec_int);
> +  int precision = MAX (mpz_sizeinbase (low, 2),
> +		       mpz_sizeinbase (up, 2));
>
>     if (precision>  BITS_PER_WORD)
>       {
> @@ -452,14 +397,10 @@ gcc_type_for_interval (mpz_t low, mpz_t up)
>         return integer_type_node;
>       }
>
> -  if (mpz_sgn (low)<= 0)
> -    unsigned_p = false;
> -
> -  else if (precision<  BITS_PER_WORD)
> -    {
> -      unsigned_p = false;
> -      precision++;
> -    }
> +  if (mpz_cmp (low, up)<= 0)
> +    unsigned_p = (mpz_sgn (low)>= 0);
> +  else
> +    unsigned_p = (mpz_sgn (up)>= 0);

What about?

        unsigned_p = value_min(low, up) >= 0;

(You need to move the implementation of value_min to this patch)

>
>     mode = smallest_mode_for_size (precision, MODE_INT);
>     precision = GET_MODE_PRECISION (mode);

In general the new implementation looks a lot more elegant as the old 
one. What was the problem with the old one? That low could be larger 
than up and that the calculation in precision_for_interval was incorrect 
(or at least not understandable for me)?

The rest of the patch looks good.

Cheers
Tobi
Sebastian Pop - June 30, 2011, 2:50 p.m.
On Wed, Jun 29, 2011 at 18:16, Tobias Grosser <tobias@grosser.es> wrote:
> why do we continue to call low 'low' and up 'up', if we actually just have
> two values v1 and v2 where we do not know which one is larger? I think this
> wrong and probably comes because we pass the lower loop bound to val_one and
> the upper loop bound to val_two.
>
> What about:
>
> +/* Return a type that could represent all values between VAL_ONE and
> +   VAL_TWO including VAL_ONE and VAL_TWO itself.  There is no
> +   constraint on which of the two values is larger.  */
>
>  static tree
> - gcc_type_for_interval (mpz_t low, mpz_t up)
> + gcc_type_for_interval (mpz_t val_one, mpz_t val_two)
>   {
>

Sounds good.  I will change the patch.

>> -  bool unsigned_p = true;
>> -  int precision, prec_up, prec_int;
>> +  bool unsigned_p;
>>    tree type;
>>    enum machine_mode mode;
>> -
>> -  gcc_assert (mpz_cmp (low, up)<= 0);
>> -
>> -  prec_up = precision_for_value (up);
>> -  prec_int = precision_for_interval (low, up);
>> -  precision = MAX (prec_up, prec_int);
>> +  int precision = MAX (mpz_sizeinbase (low, 2),
>> +                      mpz_sizeinbase (up, 2));
>>
>>    if (precision>  BITS_PER_WORD)
>>      {
>> @@ -452,14 +397,10 @@ gcc_type_for_interval (mpz_t low, mpz_t up)
>>        return integer_type_node;
>>      }
>>
>> -  if (mpz_sgn (low)<= 0)
>> -    unsigned_p = false;
>> -
>> -  else if (precision<  BITS_PER_WORD)
>> -    {
>> -      unsigned_p = false;
>> -      precision++;
>> -    }
>> +  if (mpz_cmp (low, up)<= 0)
>> +    unsigned_p = (mpz_sgn (low)>= 0);
>> +  else
>> +    unsigned_p = (mpz_sgn (up)>= 0);
>
> What about?
>
>       unsigned_p = value_min(low, up) >= 0;

Ok.

>
> (You need to move the implementation of value_min to this patch)
>
>>
>>    mode = smallest_mode_for_size (precision, MODE_INT);
>>    precision = GET_MODE_PRECISION (mode);
>
> In general the new implementation looks a lot more elegant as the old one.
> What was the problem with the old one? That low could be larger than up and

I don't think that could happen, given that we have a
gcc_assert (mpz_cmp (low, up)<= 0);

> that the calculation in precision_for_interval was incorrect (or at least
> not understandable for me)?

There was an off by one problem in the computation of precision exposed
by the patch "Compute LB and UB of a CLAST expression".

Sebastian
Tobias Grosser - June 30, 2011, 2:53 p.m.
On 06/30/2011 09:50 AM, Sebastian Pop wrote:
> On Wed, Jun 29, 2011 at 18:16, Tobias Grosser<tobias@grosser.es>  wrote:
>> why do we continue to call low 'low' and up 'up', if we actually just have
>> two values v1 and v2 where we do not know which one is larger? I think this
>> wrong and probably comes because we pass the lower loop bound to val_one and
>> the upper loop bound to val_two.
>>
>> What about:
>>
>> +/* Return a type that could represent all values between VAL_ONE and
>> +   VAL_TWO including VAL_ONE and VAL_TWO itself.  There is no
>> +   constraint on which of the two values is larger.  */
>>
>>   static tree
>> - gcc_type_for_interval (mpz_t low, mpz_t up)
>> + gcc_type_for_interval (mpz_t val_one, mpz_t val_two)
>>    {
>>
>
> Sounds good.  I will change the patch.
>
>>> -  bool unsigned_p = true;
>>> -  int precision, prec_up, prec_int;
>>> +  bool unsigned_p;
>>>     tree type;
>>>     enum machine_mode mode;
>>> -
>>> -  gcc_assert (mpz_cmp (low, up)<= 0);
>>> -
>>> -  prec_up = precision_for_value (up);
>>> -  prec_int = precision_for_interval (low, up);
>>> -  precision = MAX (prec_up, prec_int);
>>> +  int precision = MAX (mpz_sizeinbase (low, 2),
>>> +                      mpz_sizeinbase (up, 2));
>>>
>>>     if (precision>    BITS_PER_WORD)
>>>       {
>>> @@ -452,14 +397,10 @@ gcc_type_for_interval (mpz_t low, mpz_t up)
>>>         return integer_type_node;
>>>       }
>>>
>>> -  if (mpz_sgn (low)<= 0)
>>> -    unsigned_p = false;
>>> -
>>> -  else if (precision<    BITS_PER_WORD)
>>> -    {
>>> -      unsigned_p = false;
>>> -      precision++;
>>> -    }
>>> +  if (mpz_cmp (low, up)<= 0)
>>> +    unsigned_p = (mpz_sgn (low)>= 0);
>>> +  else
>>> +    unsigned_p = (mpz_sgn (up)>= 0);
>>
>> What about?
>>
>>        unsigned_p = value_min(low, up)>= 0;
>
> Ok.
>
>>
>> (You need to move the implementation of value_min to this patch)
>>
>>>
>>>     mode = smallest_mode_for_size (precision, MODE_INT);
>>>     precision = GET_MODE_PRECISION (mode);
>>
>> In general the new implementation looks a lot more elegant as the old one.
>> What was the problem with the old one? That low could be larger than up and
>
> I don't think that could happen, given that we have a
> gcc_assert (mpz_cmp (low, up)<= 0);
>
>> that the calculation in precision_for_interval was incorrect (or at least
>> not understandable for me)?
>
> There was an off by one problem in the computation of precision exposed
> by the patch "Compute LB and UB of a CLAST expression".

OK. From my side this patch is fine.

Tobi
H.J. Lu - July 5, 2011, 9:35 p.m.
On Wed, Jun 29, 2011 at 10:35 AM, Sebastian Pop <sebpop@gmail.com> wrote:
> 2011-06-29  Sebastian Pop  <sebastian.pop@amd.com>
>
>        * graphite-clast-to-gimple.c (precision_for_value): Removed.
>        (precision_for_interval): Removed.
>        (gcc_type_for_interval): Use mpz_sizeinbase.
> ---

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49649

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 828559a..0616b10 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,11 @@ 
 2011-06-29  Sebastian Pop  <sebastian.pop@amd.com>
 
+	* graphite-clast-to-gimple.c (precision_for_value): Removed.
+	(precision_for_interval): Removed.
+	(gcc_type_for_interval): Use mpz_sizeinbase.
+
+2011-06-29  Sebastian Pop  <sebastian.pop@amd.com>
+
 	PR tree-optimization/47654
 	* graphite-blocking.c (pbb_strip_mine_time_depth): Do not return bool.
 	(lst_do_strip_mine_loop): Return an int.
diff --git a/gcc/graphite-clast-to-gimple.c b/gcc/graphite-clast-to-gimple.c
index 4a4c3d2..70031a0 100644
--- a/gcc/graphite-clast-to-gimple.c
+++ b/gcc/graphite-clast-to-gimple.c
@@ -379,72 +379,17 @@  clast_to_gcc_expression (tree type, struct clast_expr *e,
   return NULL_TREE;
 }
 
-/* Return the precision needed to represent the value VAL.  */
-
-static int
-precision_for_value (mpz_t val)
-{
-  mpz_t x, y, two;
-  int precision;
-
-  mpz_init (x);
-  mpz_init (y);
-  mpz_init (two);
-  mpz_set_si (x, 2);
-  mpz_set (y, val);
-  mpz_set_si (two, 2);
-  precision = 1;
-
-  if (mpz_sgn (y) < 0)
-    mpz_neg (y, y);
-
-  while (mpz_cmp (y, x) >= 0)
-    {
-      mpz_mul (x, x, two);
-      precision++;
-    }
-
-  mpz_clear (x);
-  mpz_clear (y);
-  mpz_clear (two);
-
-  return precision;
-}
-
-/* Return the precision needed to represent the values between LOW and
-   UP.  */
-
-static int
-precision_for_interval (mpz_t low, mpz_t up)
-{
-  mpz_t diff;
-  int precision;
-
-  gcc_assert (mpz_cmp (low, up) <= 0);
-
-  mpz_init (diff);
-  mpz_sub (diff, up, low);
-  precision = precision_for_value (diff);
-  mpz_clear (diff);
-
-  return precision;
-}
-
-/* Return a type that could represent the integer value VAL.  */
+/* Return a type that could represent the values between LOW and UP.
+   The value of LOW can be bigger than UP.  */
 
 static tree
 gcc_type_for_interval (mpz_t low, mpz_t up)
 {
-  bool unsigned_p = true;
-  int precision, prec_up, prec_int;
+  bool unsigned_p;
   tree type;
   enum machine_mode mode;
-
-  gcc_assert (mpz_cmp (low, up) <= 0);
-
-  prec_up = precision_for_value (up);
-  prec_int = precision_for_interval (low, up);
-  precision = MAX (prec_up, prec_int);
+  int precision = MAX (mpz_sizeinbase (low, 2),
+		       mpz_sizeinbase (up, 2));
 
   if (precision > BITS_PER_WORD)
     {
@@ -452,14 +397,10 @@  gcc_type_for_interval (mpz_t low, mpz_t up)
       return integer_type_node;
     }
 
-  if (mpz_sgn (low) <= 0)
-    unsigned_p = false;
-
-  else if (precision < BITS_PER_WORD)
-    {
-      unsigned_p = false;
-      precision++;
-    }
+  if (mpz_cmp (low, up) <= 0)
+    unsigned_p = (mpz_sgn (low) >= 0);
+  else
+    unsigned_p = (mpz_sgn (up) >= 0);
 
   mode = smallest_mode_for_size (precision, MODE_INT);
   precision = GET_MODE_PRECISION (mode);