Re: [wide-int] int_traits <tree>

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

On 10/16/2013 09:55 AM, Richard Biener wrote:
> Speaking in terms of a patch:
>
> Index: tree.h
> ===================================================================
> --- tree.h      (revision 203690)
> +++ tree.h      (working copy)
> @@ -5184,14 +5184,11 @@ wi::int_traits <const_tree>::decompose (
>                            / HOST_BITS_PER_WIDE_INT);
>     unsigned int xprecision = get_precision (x);
>   
> -  gcc_assert (precision >= xprecision);
> +  gcc_checking_assert (precision >= xprecision && precision != 0);
>   
> -  /* Got to be careful of precision 0 values.  */
> -  if (precision)
> -    len = MIN (len, max_len);
>     if (TYPE_SIGN (TREE_TYPE (x)) == UNSIGNED)
>       {
> -      unsigned int small_prec = precision & (HOST_BITS_PER_WIDE_INT - 1);
> +      unsigned int small_prec = xprecision & (HOST_BITS_PER_WIDE_INT -
> 1);
>         if (small_prec)
>          {
>            /* We have to futz with this because the canonization for
> @@ -5201,7 +5198,7 @@ wi::int_traits <const_tree>::decompose (
>              {
>                for (unsigned int i = 0; i < len - 1; i++)
>                  scratch[i] = val[i];
> -             scratch[len - 1] = sext_hwi (val[len - 1], precision);
> +             scratch[len - 1] = sext_hwi (val[len - 1], small_prec);
>                return wi::storage_ref (scratch, len, precision);
>              }
>          }
>
> the precision 0 thing is gone as far as I understand?
There is still the case where the front ends create types with precision 
0, and that causes a few odd cases, but the precision 0 that you refer 
to is gone.
> Also sign-extending the upper hwi must be done with precision %
> HOST_BITS_PER_WIDE_INT, no?  And it assumes that we only ever
> have to extend the upper most HWI, thus 'len' is never too big.
yes
> I note that we immediately return in the above case, so
>
>        if (precision < xprecision + HOST_BITS_PER_WIDE_INT)
>          {
>            len = wi::force_to_size (scratch, val, len, xprecision,
> precision, UNSIGNED);
>            return wi::storage_ref (scratch, len, precision);
>          }
>
> applies only when the desired precision is a multiple of a HWI.
yes
> I assume it adds that extra zero word in case the MSB is set,
> right?
yes
> I am confused about the condition written here and
> how we look at precision and not xprecision when deciding how
> to extend - given a 8-bit precision unsigned 0xff and precision == 64
> we do not even consider sign-extending the value because we look
> at precision and not xprecision.  Don't we need to look at
> xprecision here?
I do not think so.   There are three cases here:

1) xprecision < precision:
say xprecision = 8 and precision = 32.

The number is between 0 and 255 and after canonicalization the number 
will be between 0 and 255.

2) xprecision == precision
not much to talk about here.

3) xprecision > precision

We need to loose the upper bits of the input and it does not matter what 
they were.    The only thing that we are concerned about is that the 
bits above what is now the sign bit pointed to by precision are matched 
in the bits above precision.


I do think we could change is that the "if (precision < xprecision + 
HWI)" to become "else if (...)", which will save a test.
> After all an assert precision == xprecision
> does not work in this routine.
>
> Quickly execute.exp tested only.
>
> To avoid code quality wreckage we have to implement a different way
> of allocating the 'scratch' storage of wide_int_ref_storage
> (well, ideally we wouldn't need it ...).  I thought of simply
> allocating it via alloca (), but we need to do that in all
> callers that build a wide_int_ref (eventually via a hidden
> default arg).  But as that's a template specialization of
> generic_wide_int ... so the option is to provide a function
> wrapper inside wi:: for this, like
I want richard and mike to be the people who respond to the next 
point.    I am not a c++ person and all of this storage manager layer 
stuff gives me a headache.

> template <typename T>
> wide_int_ref
> wi::ref (const T &t, HOST_WIDE_INT *scratch = XALLOCAVEC (HOST_WIDE_INT,
> WIDE_INT_MAX_ELTS))
> {
>    return wide_int_ref (t, scratch);
> }
>
> and amend the storage constructors to get a scratch argument.
>
> The reason for all this is that otherwise the wide_int_ref object
> escapes when we pass the scratch storage to the workers in
> int_traits <const_tree>.
>
> Thanks,
> Richard.