Message ID | 87ob63kt6i.fsf@talisman.default |
---|---|
State | New |
Headers | show |
I always like patches that change host dependencies into more general code. ok from my perspective. kenny On 11/02/2013 06:13 AM, Richard Sandiford wrote: > I decided to lump these together since the problems were the same. > There were some typos in the real_to_integer invocation, while changing: > > /* There must be no padding. */ > if (!host_integerp (TYPE_SIZE (type), 1) > || (tree_low_cst (TYPE_SIZE (type), 1) > != count * GET_MODE_BITSIZE (*modep))) > return -1; > > to: > > if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > || (tree_to_uhwi (TYPE_SIZE (type)) > != count * GET_MODE_BITSIZE (*modep))) > return -1; > > introduced a signed/unsigned warning. > > Tested on aarch64-linux-gnueabi & arm-linux-gnueabi and applied as > obvious. > > Thanks, > Richard > > > Index: gcc/config/aarch64/aarch64.c > =================================================================== > --- gcc/config/aarch64/aarch64.c (revision 204311) > +++ gcc/config/aarch64/aarch64.c (working copy) > @@ -6030,9 +6030,7 @@ > - tree_to_uhwi (TYPE_MIN_VALUE (index))); > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; > > return count; > @@ -6060,9 +6058,7 @@ > } > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; > > return count; > @@ -6092,9 +6088,7 @@ > } > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; > > return count; > @@ -7433,7 +7427,7 @@ > int exponent; > unsigned HOST_WIDE_INT mantissa, mask; > REAL_VALUE_TYPE r, m; > - bool &fail > + bool fail; > > if (!CONST_DOUBLE_P (x)) > return false; > @@ -7457,7 +7451,7 @@ > WARNING: If we ever have a representation using more than 2 * H_W_I - 1 > bits for the mantissa, this can fail (low bits will be lost). */ > real_ldexp (&m, &r, point_pos - exponent); > - w = real_to_integer (m, &fail, HOST_BITS_PER_WIDE_INT * 2); > + wide_int w = real_to_integer (&m, &fail, HOST_BITS_PER_WIDE_INT * 2); > > /* If the low part of the mantissa has bits set we cannot represent > the value. */ > Index: gcc/config/arm/arm.c > =================================================================== > --- gcc/config/arm/arm.c (revision 204311) > +++ gcc/config/arm/arm.c (working copy) > @@ -4678,9 +4678,7 @@ > - tree_to_uhwi (TYPE_MIN_VALUE (index))); > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; > > return count; > @@ -4708,9 +4706,7 @@ > } > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; > > return count; > @@ -4740,9 +4736,7 @@ > } > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; > > return count; > @@ -11299,7 +11293,7 @@ > WARNING: If there's ever a VFP version which uses more than 2 * H_W_I - 1 > bits for the mantissa, this may fail (low bits would be lost). */ > real_ldexp (&m, &r, point_pos - exponent); > - wide_int w = real_to_integer (m, &fail, HOST_BITS_PER_WIDE_INT * 2); > + wide_int w = real_to_integer (&m, &fail, HOST_BITS_PER_WIDE_INT * 2); > mantissa = w.elt (0); > mant_hi = w.elt (1); >
On Nov 2, 2013, at 3:13 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > I decided to lump these together since the problems were the same. > There were some typos in the real_to_integer invocation, while changing: > > /* There must be no padding. */ > if (!host_integerp (TYPE_SIZE (type), 1) > || (tree_low_cst (TYPE_SIZE (type), 1) > != count * GET_MODE_BITSIZE (*modep))) > return -1; > > to: > > if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > || (tree_to_uhwi (TYPE_SIZE (type)) > != count * GET_MODE_BITSIZE (*modep))) > return -1; > > introduced a signed/unsigned warning. > > Tested on aarch64-linux-gnueabi & arm-linux-gnueabi and applied as > obvious. > > Thanks, > Richard > > > Index: gcc/config/aarch64/aarch64.c > =================================================================== > --- gcc/config/aarch64/aarch64.c (revision 204311) > +++ gcc/config/aarch64/aarch64.c (working copy) > @@ -6030,9 +6030,7 @@ > - tree_to_uhwi (TYPE_MIN_VALUE (index))); > > /* There must be no padding. */ > - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) > - || (tree_to_uhwi (TYPE_SIZE (type)) > - != count * GET_MODE_BITSIZE (*modep))) > + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) > return -1; So, one of the review comments concerns this type of change. The specific comment was from David on rs6000 point #5. My (our) question is, doesn't Ada have non-INTEGER_CST TYPE_SIZE (type), and the old code had this type of check: bool tree_fits_uhwi_p (const_tree t) { return (t != NULL_TREE && TREE_CODE (t) == INTEGER_CST && TREE_INT_CST_HIGH (t) == 0); } to ensure that things that are not INTEGER_CSTs return -1. In the new code, won't this just call wi::ne_p, and die? I'm not an Ada person, so, I don't know if my fears are founded, and I don't claim to know all the checks that happen in the callers before this point.
Mike Stump <mikestump@comcast.net> writes: > On Nov 2, 2013, at 3:13 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> I decided to lump these together since the problems were the same. >> There were some typos in the real_to_integer invocation, while changing: >> >> /* There must be no padding. */ >> if (!host_integerp (TYPE_SIZE (type), 1) >> || (tree_low_cst (TYPE_SIZE (type), 1) >> != count * GET_MODE_BITSIZE (*modep))) >> return -1; >> >> to: >> >> if (!tree_fits_uhwi_p (TYPE_SIZE (type)) >> || (tree_to_uhwi (TYPE_SIZE (type)) >> != count * GET_MODE_BITSIZE (*modep))) >> return -1; >> >> introduced a signed/unsigned warning. >> >> Tested on aarch64-linux-gnueabi & arm-linux-gnueabi and applied as >> obvious. >> >> Thanks, >> Richard >> >> >> Index: gcc/config/aarch64/aarch64.c >> =================================================================== >> --- gcc/config/aarch64/aarch64.c (revision 204311) >> +++ gcc/config/aarch64/aarch64.c (working copy) >> @@ -6030,9 +6030,7 @@ >> - tree_to_uhwi (TYPE_MIN_VALUE (index))); >> >> /* There must be no padding. */ >> - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) >> - || (tree_to_uhwi (TYPE_SIZE (type)) >> - != count * GET_MODE_BITSIZE (*modep))) >> + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) >> return -1; > > So, one of the review comments concerns this type of change. The > specific comment was from David on rs6000 point #5. > > My (our) question is, doesn't Ada have non-INTEGER_CST TYPE_SIZE > (type), and the old code had this type of check: > > bool > tree_fits_uhwi_p (const_tree t) > { > return (t != NULL_TREE > && TREE_CODE (t) == INTEGER_CST > && TREE_INT_CST_HIGH (t) == 0); > } > > to ensure that things that are not INTEGER_CSTs return -1. In the new > code, won't this just call wi::ne_p, and die? Ah, sorry, hadn't realised that was possible here. In that case I agree we should keep the INTEGER_CST check. I.e.: if (TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST || wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; Thanks, Richard
> My (our) question is, doesn't Ada have non-INTEGER_CST TYPE_SIZE (type), and > the old code had this type of check: > > bool > tree_fits_uhwi_p (const_tree t) > { > return (t != NULL_TREE > && TREE_CODE (t) == INTEGER_CST > && TREE_INT_CST_HIGH (t) == 0); > } > > to ensure that things that are not INTEGER_CSTs return -1. In the new code, > won't this just call wi::ne_p, and die? TYPE_SIZE (type) can be anything even in C: int foo (int i, int j) { char a[i *2 + j + 1]; a [0] = 1; } (gdb) p debug_generic_expr(type->type_common.size) (bitsizetype) (sizetype) (SAVE_EXPR <(i * 2 + j) + 1>) * 8
Index: gcc/config/aarch64/aarch64.c =================================================================== --- gcc/config/aarch64/aarch64.c (revision 204311) +++ gcc/config/aarch64/aarch64.c (working copy) @@ -6030,9 +6030,7 @@ - tree_to_uhwi (TYPE_MIN_VALUE (index))); /* There must be no padding. */ - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) - || (tree_to_uhwi (TYPE_SIZE (type)) - != count * GET_MODE_BITSIZE (*modep))) + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; return count; @@ -6060,9 +6058,7 @@ } /* There must be no padding. */ - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) - || (tree_to_uhwi (TYPE_SIZE (type)) - != count * GET_MODE_BITSIZE (*modep))) + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; return count; @@ -6092,9 +6088,7 @@ } /* There must be no padding. */ - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) - || (tree_to_uhwi (TYPE_SIZE (type)) - != count * GET_MODE_BITSIZE (*modep))) + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; return count; @@ -7433,7 +7427,7 @@ int exponent; unsigned HOST_WIDE_INT mantissa, mask; REAL_VALUE_TYPE r, m; - bool &fail + bool fail; if (!CONST_DOUBLE_P (x)) return false; @@ -7457,7 +7451,7 @@ WARNING: If we ever have a representation using more than 2 * H_W_I - 1 bits for the mantissa, this can fail (low bits will be lost). */ real_ldexp (&m, &r, point_pos - exponent); - w = real_to_integer (m, &fail, HOST_BITS_PER_WIDE_INT * 2); + wide_int w = real_to_integer (&m, &fail, HOST_BITS_PER_WIDE_INT * 2); /* If the low part of the mantissa has bits set we cannot represent the value. */ Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c (revision 204311) +++ gcc/config/arm/arm.c (working copy) @@ -4678,9 +4678,7 @@ - tree_to_uhwi (TYPE_MIN_VALUE (index))); /* There must be no padding. */ - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) - || (tree_to_uhwi (TYPE_SIZE (type)) - != count * GET_MODE_BITSIZE (*modep))) + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; return count; @@ -4708,9 +4706,7 @@ } /* There must be no padding. */ - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) - || (tree_to_uhwi (TYPE_SIZE (type)) - != count * GET_MODE_BITSIZE (*modep))) + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; return count; @@ -4740,9 +4736,7 @@ } /* There must be no padding. */ - if (!tree_fits_uhwi_p (TYPE_SIZE (type)) - || (tree_to_uhwi (TYPE_SIZE (type)) - != count * GET_MODE_BITSIZE (*modep))) + if (wi::ne_p (TYPE_SIZE (type), count * GET_MODE_BITSIZE (*modep))) return -1; return count; @@ -11299,7 +11293,7 @@ WARNING: If there's ever a VFP version which uses more than 2 * H_W_I - 1 bits for the mantissa, this may fail (low bits would be lost). */ real_ldexp (&m, &r, point_pos - exponent); - wide_int w = real_to_integer (m, &fail, HOST_BITS_PER_WIDE_INT * 2); + wide_int w = real_to_integer (&m, &fail, HOST_BITS_PER_WIDE_INT * 2); mantissa = w.elt (0); mant_hi = w.elt (1);