wide-int, gimple

On Sat, Nov 23, 2013 at 8:21 PM, Mike Stump <mikestump@comcast.net> wrote:
> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch.    This patch covers the gimple code.
>
> Ok?

@@ -1754,7 +1754,7 @@ dump_ssaname_info (pretty_printer *buffer, tree
node, int spc)
   if (!POINTER_TYPE_P (TREE_TYPE (node))
       && SSA_NAME_RANGE_INFO (node))
     {
-      double_int min, max, nonzero_bits;
+      widest_int min, max, nonzero_bits;
       value_range_type range_type = get_range_info (node, &min, &max);

       if (range_type == VR_VARYING)

this makes me suspect you are changing SSA_NAME_RANGE_INFO
to embed two max wide_ints.  That's a no-no.


that's similarly excessive.  You've said widest_int is only for automatic
use (thus, stack allocated).  Yet they sneak in into basic structures
of optimization passes ...

The above is a case where a pass should decide on a limit.  For
SLSR an offset_int is appropriate (but yes, you then need to retain
or even add checks whether uses fit).  In some cases making
wide_int the trailing element in pass local data can allow for
more optimal dynamic allocation as well.

@@ -831,9 +831,17 @@ gimple_divmod_fixed_value_transform
(gimple_stmt_iterator *si)
   else
     prob = 0;

-  tree_val = build_int_cst_wide (get_gcov_type (),
-                                (unsigned HOST_WIDE_INT) val,
-                                val >> (HOST_BITS_PER_WIDE_INT - 1) >> 1);
+  if (sizeof (gcov_type) == sizeof (HOST_WIDE_INT))
+    tree_val = build_int_cst (get_gcov_type (), val);
+  else
+    {
+      HOST_WIDE_INT a[2];
+      a[0] = (unsigned HOST_WIDE_INT) val;
+      a[1] = val >> (HOST_BITS_PER_WIDE_INT - 1) >> 1;
+
+      tree_val = wide_int_to_tree (get_gcov_type (),
wide_int::from_array (a, 2,
+       TYPE_PRECISION (get_gcov_type ()), false));
+    }

is two times replicated.  Please add a build_int_cst overload for
HOST_WIDEST_INT instead (conditional on HOST_WIDEST_INT != HOST_WIDE_INT).

Thanks,
Richard.

On Mon, Nov 25, 2013 at 12:24:30PM +0100, Richard Biener wrote:
> On Sat, Nov 23, 2013 at 8:21 PM, Mike Stump <mikestump@comcast.net> wrote:
> > Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch.    This patch covers the gimple code.
> 
> @@ -1754,7 +1754,7 @@ dump_ssaname_info (pretty_printer *buffer, tree
> node, int spc)
>    if (!POINTER_TYPE_P (TREE_TYPE (node))
>        && SSA_NAME_RANGE_INFO (node))
>      {
> -      double_int min, max, nonzero_bits;
> +      widest_int min, max, nonzero_bits;
>        value_range_type range_type = get_range_info (node, &min, &max);
> 
>        if (range_type == VR_VARYING)
> 
> this makes me suspect you are changing SSA_NAME_RANGE_INFO
> to embed two max wide_ints.  That's a no-no.

Well, the range_info_def struct right now contains 3 double_ints, which is
unnecessary overhead for the most of the cases where the SSA_NAME's type
has just at most HOST_BITS_PER_WIDE_INT bits and thus we could fit all 3 of
them into 3 HOST_WIDE_INTs rather than 3 double_ints.  So supposedly struct
range_info_def could be a template on the type's precision rounded up to HWI
bits, or say have 3 alternatives there, use
FIXED_WIDE_INT (HOST_BITS_PER_WIDE_INT) for the smallest types,
FIXED_WIDE_INT (2 * HOST_BITS_PER_WIDE_INT) aka double_int for the larger
but still common ones, and widest_int for the rest, then the API to set/get
it could use widest_int everywhere, and just what storage we'd use would
depend on the precision of the type.

	Jakub

Jakub Jelinek <jakub@redhat.com> writes:
> On Mon, Nov 25, 2013 at 12:24:30PM +0100, Richard Biener wrote:
>> On Sat, Nov 23, 2013 at 8:21 PM, Mike Stump <mikestump@comcast.net> wrote:
>> > Richi has asked the we break the wide-int patch so that the
>> > individual port and front end maintainers can review their parts
>> > without have to go through the entire patch.  This patch covers the
>> > gimple code.
>> 
>> @@ -1754,7 +1754,7 @@ dump_ssaname_info (pretty_printer *buffer, tree
>> node, int spc)
>>    if (!POINTER_TYPE_P (TREE_TYPE (node))
>>        && SSA_NAME_RANGE_INFO (node))
>>      {
>> -      double_int min, max, nonzero_bits;
>> +      widest_int min, max, nonzero_bits;
>>        value_range_type range_type = get_range_info (node, &min, &max);
>> 
>>        if (range_type == VR_VARYING)
>> 
>> this makes me suspect you are changing SSA_NAME_RANGE_INFO
>> to embed two max wide_ints.  That's a no-no.
>
> Well, the range_info_def struct right now contains 3 double_ints, which is
> unnecessary overhead for the most of the cases where the SSA_NAME's type
> has just at most HOST_BITS_PER_WIDE_INT bits and thus we could fit all 3 of
> them into 3 HOST_WIDE_INTs rather than 3 double_ints.  So supposedly struct
> range_info_def could be a template on the type's precision rounded up to HWI
> bits, or say have 3 alternatives there, use
> FIXED_WIDE_INT (HOST_BITS_PER_WIDE_INT) for the smallest types,
> FIXED_WIDE_INT (2 * HOST_BITS_PER_WIDE_INT) aka double_int for the larger
> but still common ones, and widest_int for the rest, then the API to set/get
> it could use widest_int everywhere, and just what storage we'd use would
> depend on the precision of the type.

Would it make sense to just use trees for the min and max?
It looks from a quick grep like set_range_info is called with trees or
range_info_def fields for all cases except anti-ranges.  Maybe for those
we could steal a bit from somewhere in the SSA_NAME structure?  From the
comments it looks like static_flag might be free.

Of course, that means polluting a different cache line when switching to
and from anti ranges, but I'd hope that's still cheaper than adding when
writing anti-ranges and subtracting when reading.

Thanks,
Richard

On Mon, Nov 25, 2013 at 09:16:24PM +0000, Richard Sandiford wrote:
> > Well, the range_info_def struct right now contains 3 double_ints, which is
> > unnecessary overhead for the most of the cases where the SSA_NAME's type
> > has just at most HOST_BITS_PER_WIDE_INT bits and thus we could fit all 3 of
> > them into 3 HOST_WIDE_INTs rather than 3 double_ints.  So supposedly struct
> > range_info_def could be a template on the type's precision rounded up to HWI
> > bits, or say have 3 alternatives there, use
> > FIXED_WIDE_INT (HOST_BITS_PER_WIDE_INT) for the smallest types,
> > FIXED_WIDE_INT (2 * HOST_BITS_PER_WIDE_INT) aka double_int for the larger
> > but still common ones, and widest_int for the rest, then the API to set/get
> > it could use widest_int everywhere, and just what storage we'd use would
> > depend on the precision of the type.
> 
> Would it make sense to just use trees for the min and max?

Note, we don't have just min/max, but also nonzero_bits.

Anyway, I don't think that is a good idea, you can fairly often end up with
values that can't be shared with something else, and the overhead of
INTEGER_CST is quite large then.

range_info_def could very well be just struct with HOST_WIDE_INT flexible
array member or just array of HOST_WIDE_INTs, all the routines that care
about the internal details of the representation are in tree-ssanames.c
- [sg]et_range_info, [sg]et_nonzero_bits and
duplicate_ssa_name_range_info/duplicate_ssa_name_fn.  All of those
work solely for INTEGRAL_TYPE_P tree types of SSA_NAMEs, and all the
functions have access to the SSA_NAME from which you can find out the
precision.

	Jakub