Re: Some wide-int review comments

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

Committed with all your changes as revision 205049.

the only interesting change is the response to your last comment.

I changed the frag to:

   /* Third, unsigned integers with top bit set never fit signed types.  */
-  if (!TYPE_UNSIGNED (type) && sgn_c == UNSIGNED && wi::neg_p (c))
-    return false;
+  if (!TYPE_UNSIGNED (type) && sgn_c == UNSIGNED)
+    {
+      int uprec = GET_MODE_PRECISION (TYPE_MODE TREE_TYPE (c));
+      if (uprec < TYPE_PRECISION (TREE_TYPE (c)))
+    {
+      /* When a tree_cst is converted to a wide-int, the precision
+         is taken from the type.  However, if the precision of the
+         mode underneath the type is smaller than that, it is
+         possible that the value will not fit.  The test below
+         fails if any bit is set between the sign bit of the
+         underlying mode and the top bit of the type.  */
+      if (wi::ne_p (wi::zext (c, uprec - 1), c))
+        return false;
+    }
+      else if (wi::neg_p (c))
+    return false;
+    }


On 11/18/2013 04:29 AM, Richard Sandiford wrote:
> Thanks for the changes.
>
> > @@ -8162,7 +8162,7 @@ fold_builtin_logarithm (location_t loc,
> >   	    /* Prepare to do logN(exp10(exponent) -> exponent*logN(10).  */
> >   	    {
> >   	      REAL_VALUE_TYPE dconst10;
> > -	      real_from_integer (&dconst10, VOIDmode, 10, SIGNED);
> > +	      real_from_integer (&dconst10, VOIDmode, wi::shwi (10, 32), SIGNED);
> >   	      x = build_real (type, dconst10);
> >   	    }
> >   	    exponent = CALL_EXPR_ARG (arg, 0);
> > @@ -8315,7 +8315,7 @@ fold_builtin_pow (location_t loc, tree f
> >   
> >         /* Check for an integer exponent.  */
> >         n = real_to_integer (&c);
> > -      real_from_integer (&cint, VOIDmode, n, SIGNED);
> > +      real_from_integer (&cint, VOIDmode, wi::shwi (n, HOST_BITS_PER_WIDE_INT), SIGNED);
> >         if (real_identical (&c, &cint))
> >   	{
> >   	  /* Attempt to evaluate pow at compile-time, unless this should
> Are these changes necessary?  The original calls ought to work as-is,
> since the function takes a const wide_int_ref &.  Same for the rest of
> the patch.
>
> > Index: gcc/c/c-parser.c
> > ===================================================================
> > --- gcc/c/c-parser.c	(revision 204918)
> > +++ gcc/c/c-parser.c	(working copy)
> > @@ -13375,7 +13375,7 @@ c_parser_cilk_clause_vectorlength (c_par
> >         || !TREE_CONSTANT (expr)
> >         || !INTEGRAL_TYPE_P (TREE_TYPE (expr)))
> >       error_at (loc, "vectorlength must be an integer constant");
> > -  else if (exact_log2 (tree_to_hwi (expr)) == -1)
> > +  else if (wi::eq_p (wi::exact_log2 (expr), -1))
> >       error_at (loc, "vectorlength must be a power of 2");
> >     else
> >       {
> FWIW:
>
>    wi::exact_log2 (expr) == -1
>
> should still work.
>
> > Index: gcc/dwarf2out.c
> > ===================================================================
> > --- gcc/dwarf2out.c	(revision 204918)
> > +++ gcc/dwarf2out.c	(working copy)
> > @@ -13428,8 +13428,6 @@ loc_descriptor (rtx rtl, enum machine_mo
> >   
> >         if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict))
> >   	{
> > -	  gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE (rtl));
> > -
> >   	  /* Note that a CONST_DOUBLE rtx could represent either an integer
> >   	     or a floating-point constant.  A CONST_DOUBLE is used whenever
> >   	     the constant requires more than one word in order to be
> The copy of the CONST_DOUBLE comment is still there though.
> (This is handling CONST_WIDE_INT rather than CONST_DOUBLE.)
>
> > Index: gcc/tree.c
> > ===================================================================
> > --- gcc/tree.c	(revision 204918)
> > +++ gcc/tree.c	(working copy)
> > @@ -8536,8 +8536,18 @@ retry:
> >       return true;
> >   
> >     /* Third, unsigned integers with top bit set never fit signed types.  */
> > -  if (!TYPE_UNSIGNED (type) && sgn_c == UNSIGNED && wi::neg_p (c))
> > -    return false;
> > +  if (!TYPE_UNSIGNED (type) && sgn_c == UNSIGNED)
> > +    {
> > +      int uprec = GET_MODE_PRECISION (TYPE_MODE TREE_TYPE (c));
> > +      if (uprec < TYPE_PRECISION (TREE_TYPE (c)))
> > +	{
> > +	  wide_int x = wi::sext (c, uprec);
> > +	  if (wi::neg_p (x) || wi::ne_p (x, c))
> > +	    return false;
> > +	}
> > +      else if (wi::neg_p (c))
> > +	return false;
> > +    }
> >   
> >     /* If we haven't been able to decide at this point, there nothing more we
> >        can check ourselves here.  Look at the base type if we have one and it
> I don't really understand this change, but I suppose it's part of
> the trunk patch.
>
> Looks good to me otherwise FWIW.
>
> Thanks,
> Richard