| Message ID | 1309368922-25238-5-git-send-email-sebpop@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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);