Re: [PATCH 4/6] Fix computation of precision.
On Wed, Jun 29, 2011 at 10:35 AM, Sebastian Pop seb...@gmail.com wrote: 2011-06-29 Sebastian Pop sebastian@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 -- H.J.
Re: [PATCH 4/6] Fix computation of precision.
On Wed, Jun 29, 2011 at 18:16, Tobias Grosser tob...@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
Re: [PATCH 4/6] Fix computation of precision.
On 06/30/2011 09:50 AM, Sebastian Pop wrote:
On Wed, Jun 29, 2011 at 18:16, Tobias Grossertob...@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 (precisionBITS_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 (precisionBITS_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
[PATCH 4/6] Fix computation of precision.
2011-06-29 Sebastian Pop sebastian@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(-)
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@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@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);
--
1.7.4.1
Re: [PATCH 4/6] Fix computation of precision.
On 06/29/2011 12:35 PM, Sebastian Pop wrote:
2011-06-29 Sebastian Popsebastian@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
