On Wed, 18 Sep 2013, Kugan wrote:
>
> Thanks Richard for the review.
> On 16/09/13 23:43, Richard Biener wrote:
> > On Mon, 16 Sep 2013, Kugan wrote:
> >
> > > Hi,
> > >
> > > Updated the patch to the latest changes in trunk that splits tree.h. I
> > > also
> > > noticed an error in printing double_int and fixed it.
> > >
> > > Is this OK?
> >
> > print_gimple_stmt (dump_file, stmt, 0,
> > - TDF_SLIM | (dump_flags & TDF_LINENO));
> > + TDF_SLIM | TDF_RANGE | (dump_flags &
> > TDF_LINENO));
> >
> > this should be (dump_flags & (TDF_LINENO|TDF_RANGE)) do not always
> > dump range info. I'd have simply re-used TDF_ALIAS (and interpret
> > it as SSA annotation info), adding -range in dump file modifiers
> > is ok with me.
> >
> > +static void
> > +print_double_int (pretty_printer *buffer, double_int cst)
> > +{
> > + tree node = double_int_to_tree (integer_type_node, cst);
> > + if (TREE_INT_CST_HIGH (node) == 0)
> > + pp_printf (buffer, HOST_WIDE_INT_PRINT_UNSIGNED, TREE_INT_CST_LOW
> > (node));
> > + else if (TREE_INT_CST_HIGH (node) == -1
> > + && TREE_INT_CST_LOW (node) != 0)
> > + pp_printf (buffer, "-" HOST_WIDE_INT_PRINT_UNSIGNED,
> > + -TREE_INT_CST_LOW (node));
> > + else
> > + sprintf (pp_buffer (buffer)->digit_buffer,
> > + HOST_WIDE_INT_PRINT_DOUBLE_HEX,
> > + (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (node),
> > + (unsigned HOST_WIDE_INT) TREE_INT_CST_LOW (node));
> >
> > using sprintf here looks like a layering violation to me. You
> > probably want to factor out code from the INTEGER_CST handling
> > of tree-pretty-print.c:dump_generic_node into a pp_double_int
> > function in pretty-print.[ch] instead.
> >
> > @@ -1628,6 +1647,27 @@ dump_gimple_phi (pretty_printer *buffer, gimple
> > phi, int spc, int flags)
> > pp_string (buffer, "# ");
> > }
> >
> > + if ((flags & TDF_RANGE)
> > + && !POINTER_TYPE_P (TREE_TYPE (lhs))
> > + && SSA_NAME_RANGE_INFO (lhs))
> > + {
> > + double_int min, max;
> > + value_range_type range_type;
> >
> > I realize the scheme is pre-existing but can you try factoring
> > out the dumping of SSA_NAME_PTR_INFO / SSA_NAME_RANGE_INFO into
> > a separate routine that can be shared by dump_gimple_phi and
> > pp_gimple_stmt_1?
> >
> > +get_range_info (tree name, double_int &min, double_int &max,
> > + enum value_range_type &range_type)
> > +{
> > + gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
> > + gcc_assert (TREE_CODE (name) == SSA_NAME);
> > + range_info_def *ri = SSA_NAME_RANGE_INFO (name);
> >
> > the TREE_CODE (name) == SSA_NAME assert is redundant with the
> > tree-checking performed by SSA_NAME_RANGE_INFO. Likewise in
> > the other functions.
> >
> > +void
> > +get_range_info (tree name, double_int &min, double_int &max,
> > + enum value_range_type &range_type)
> >
> > I'm not sure we want to use references. Well - first time.
> >
> > + /* If min > max, it is VR_ANTI_RANGE. */
> > + if (ri->min.scmp (ri->max) == 1)
> > + {
> >
> > I think that's wrong and needs to be conditional on TYPE_UNSIGNED
> > of the SSA name.
> >
> > + else if (vr_value[i]->type == VR_ANTI_RANGE)
> > + {
> > + /* VR_ANTI_RANGE ~[min, max] is encoded compactly as
> > + [max + 1, min - 1] without additional attributes.
> > + When min value > max value, we know that it is
> > + VR_ANTI_RANGE; it is VR_RANGE othewise. */
> > + set_range_info (name,
> > + tree_to_double_int (vr_value[i]->max)
> > + + double_int_one,
> > + tree_to_double_int (vr_value[i]->min)
> > + - double_int_one);
> >
> > there is a complication for when max + 1 or min - 1 overflow - those
> > should be non-canonical ranges I think, but double-check this
> > (check set_and_canonicalize_value_range).
> >
> I have now added a check for min == 0 for unsigned type. AFAIK, For double_int
> type, this is the only case we should check.
>
> I have also made the other changes you have asked me to do. Please find the
> modified patch and ChangeLog.
>
> Bootstrapped and regtested for x86_64-unknown-linux-gnu. Is this OK.
>
> Thanks,
> Kugan
>
>
> +2013-09-17 Kugan Vivekanandarajah <kug...@linaro.org>
> +
> + * gimple-pretty-print.c (dump_ssaname_info) : New function.
> + * gimple-pretty-print.c (dump_gimple_phi) : Dump range info.
> + * (pp_gimple_stmt_1) : Likewise.
ChangeLog should be formated
* gimple-pretty-print.c (dump_ssaname_info): New function.
(dump_gimple_phi): Call it.
(pp_gimple_stmt_1: Likewise.
* tree-pretty-print.c (dump_intger_cst_node): New function.
...
+ pp_printf (buffer, "# RANGE ");
+ pp_printf (buffer, "%s[", range_type == VR_RANGE ? "" : "~");
+ dump_intger_cst_node (buffer,
+ double_int_to_tree (TREE_TYPE (node),
min));
I was asking for a pp_double_int, not a dump_integer_cst_node function
as now you are creating a tree node in GC memory just to dump its
contents ... pp_double_int needs to be passed information on the
signedness of the value. It would roughly look like
pp_double_int (pretty_printer *pp, double_int d, bool uns)
{
if (d.fits_shwi ())
pp_wide_integer (pp, d.low);
else if (d.fits_uhwi ())
pp_unsigned_wide_integer (pp, d.low);
else
{
unsigned HOST_WIDE_INT low = d.low;
HOST_WIDE_INT high = d.high;
if (!uns && d.is_negative ())
{
pp_minus (pp);
high = ~high + !low;
low = -low;
}
/* Would "%x%0*x" or "%x%*0x" get zero-padding on all
systems? */
sprintf (pp_buffer (pp)->digit_buffer,
HOST_WIDE_INT_PRINT_DOUBLE_HEX,
(unsigned HOST_WIDE_INT) high, low);
pp_string (pp, pp_buffer (pp)->digit_buffer);
}
}
and the INTEGER_CST case would use it like
if (TREE_CODE (TREE_TYPE (node)) == POINTER_TYPE)
...
else
pp_double_int (buffer, tree_to_double_int (node),
TYPE_UNSIGNED (TREE_TYPE (node)));
+enum value_range_type
+get_range_info (tree name, double_int *min, double_int *max)
+{
ah, I see you have already made an appropriate change here.
+ /* Check for an empty range with minimum zero (of type
+ unsigned) that will wraparround. */
+ if (!(TYPE_UNSIGNED (TREE_TYPE (name))
+ && integer_zerop (vr_value[i]->min)))
+ set_range_info (name,
+ tree_to_double_int (vr_value[i]->max)
+ + double_int_one,
+ tree_to_double_int (vr_value[i]->min)
+ - double_int_one);
Yeah, I think ~[0,0] is the only anti-range that can be represented as
range that we keep. So maybe
if (TYPE_UNSIGNED (TREE_TYPE (name))
&& integer_zerop (vr_value[i]->min)
&& integer_zerop (vr_value[i]->max))
set_range_info (name,
double_int_one,
double_int::max_value
(TYPE_PRECISION (TREE_TYPE (name)), true));
else
set_range_info (name,
+ tree_to_double_int (vr_value[i]->max)
+ + double_int_one,
+ tree_to_double_int (vr_value[i]->min)
+ - double_int_one);
to preserve ~[0,0] which looks like an important case when for example
looking at a divisor in a division.
Ok with those changes.
Thanks,
Richard.
k
