Richard Sandiford <rdsandif...@googlemail.com> writes:
> This code was mixing hprec and hprec*2 wide_ints. The simplest fix
> seemed to be to introduce a function that gives the minimum precision
> necessary to represent a function, which also means that no temporary
> wide_ints are needed.
>
> Other places might be able to use this too, but I'd like to look at
> that after the merge.
>
> The patch series fixed a regression in c-c++-common/ubsan/overflow-2.c
> and I assume it's due to this change.
>
> Tested on x86_64-linux-gnu. OK to install?
Richard B. expressed doubts about this on IRC, so for a bit more detail:
The comparisons we're doing are on the range of an SSA name.
There are three ways that these ranges could be stored in the
range_info_def:
(1) as INTEGER_CSTs. This was felt to be unacceptable because it would
create too many garbage constants.
(2) as widest_ints. This too was unacceptable because it would bloat
the range_info_def.
(3) as a form of wide_int in which the HWIs are allocated as a trailing
part of the containing structure. This means that range_info_defs
for 64-bit types only have 3 HWIs (smaller than now).
We went for (3).
Having decided to store the ranges like wide_ints, the question then is:
what about the get/set_range_info interface? Two obvious options are:
(a) present the ranges as wide_ints.
(b) present the ranges as widest_ints, converting in and out as necessary.
(a) is more efficient and seems to fit well with the pre-ubsan callers,
so that's what was chosen.
In the patch we have two wide_ints that have the same precision as the
SSA name. The values we were creating via wi::min_value and wi::max_value
instead had half that precision. This is the same kind of mismatch as
you'd get comparing HImode and SImode in RTL, say.
We could fix the bug by using something like:
if (wi::les_p (arg0_max, wi::mask (hprec, false, prec))
&& wi::les_p (wi::mask (hprec, true, prec), arg0_min))
etc. Or we could extend the wide_ints to widest_ints so that "precision
doesn't matter" when doing the comparisons. But both those options
involve temporaries and seem unnecessarily complicated. All we're
really asking here is: what is the minimum precision needed to represent
this constant? That's something that could be generally useful
(e.g. when checking whether a value fits a type) so the patch adds
a corresponding wi:: function.
Thanks,
Richard
>
> Thanks,
> Richard
>
>
> Index: gcc/internal-fn.c
> ===================================================================
> --- gcc/internal-fn.c 2014-04-22 20:31:10.516895118 +0100
> +++ gcc/internal-fn.c 2014-04-22 20:31:25.842005530 +0100
> @@ -478,7 +478,7 @@ ubsan_expand_si_overflow_mul_check (gimp
> rtx do_overflow = gen_label_rtx ();
> rtx hipart_different = gen_label_rtx ();
>
> - int hprec = GET_MODE_PRECISION (hmode);
> + unsigned int hprec = GET_MODE_PRECISION (hmode);
> rtx hipart0 = expand_shift (RSHIFT_EXPR, mode, op0, hprec,
> NULL_RTX, 0);
> hipart0 = gen_lowpart (hmode, hipart0);
> @@ -513,12 +513,11 @@ ubsan_expand_si_overflow_mul_check (gimp
> wide_int arg0_min, arg0_max;
> if (get_range_info (arg0, &arg0_min, &arg0_max) == VR_RANGE)
> {
> - if (wi::les_p (arg0_max, wi::max_value (hprec, SIGNED))
> - && wi::les_p (wi::min_value (hprec, SIGNED), arg0_min))
> + unsigned int mprec0 = wi::min_precision (arg0_min, SIGNED);
> + unsigned int mprec1 = wi::min_precision (arg0_max, SIGNED);
> + if (mprec0 <= hprec && mprec1 <= hprec)
> op0_small_p = true;
> - else if (wi::les_p (arg0_max, wi::max_value (hprec, UNSIGNED))
> - && wi::les_p (~wi::max_value (hprec, UNSIGNED),
> - arg0_min))
> + else if (mprec0 <= hprec + 1 && mprec1 <= hprec + 1)
> op0_medium_p = true;
> if (!wi::neg_p (arg0_min, TYPE_SIGN (TREE_TYPE (arg0))))
> op0_sign = 0;
> @@ -531,12 +530,11 @@ ubsan_expand_si_overflow_mul_check (gimp
> wide_int arg1_min, arg1_max;
> if (get_range_info (arg1, &arg1_min, &arg1_max) == VR_RANGE)
> {
> - if (wi::les_p (arg1_max, wi::max_value (hprec, SIGNED))
> - && wi::les_p (wi::min_value (hprec, SIGNED), arg1_min))
> + unsigned int mprec0 = wi::min_precision (arg1_min, SIGNED);
> + unsigned int mprec1 = wi::min_precision (arg1_max, SIGNED);
> + if (mprec0 <= hprec && mprec1 <= hprec)
> op1_small_p = true;
> - else if (wi::les_p (arg1_max, wi::max_value (hprec, UNSIGNED))
> - && wi::les_p (~wi::max_value (hprec, UNSIGNED),
> - arg1_min))
> + else if (mprec0 <= hprec + 1 && mprec1 <= hprec + 1)
> op1_medium_p = true;
> if (!wi::neg_p (arg1_min, TYPE_SIGN (TREE_TYPE (arg1))))
> op1_sign = 0;
> Index: gcc/wide-int.h
> ===================================================================
> --- gcc/wide-int.h 2014-04-22 20:31:10.516895118 +0100
> +++ gcc/wide-int.h 2014-04-22 20:31:25.842005530 +0100
> @@ -562,6 +562,9 @@ #define SHIFT_FUNCTION \
>
> template <typename T>
> unsigned HOST_WIDE_INT extract_uhwi (const T &, unsigned int, unsigned
> int);
> +
> + template <typename T>
> + unsigned int min_precision (const T &, signop);
> }
>
> namespace wi
> @@ -2995,6 +2998,17 @@ wi::extract_uhwi (const T &x, unsigned i
> return zext_hwi (res, width);
> }
>
> +/* Return the minimum precision needed to store X with sign SGN. */
> +template <typename T>
> +inline unsigned int
> +wi::min_precision (const T &x, signop sgn)
> +{
> + if (sgn == SIGNED)
> + return wi::get_precision (x) - clrsb (x);
> + else
> + return wi::get_precision (x) - clz (x);
> +}
> +
> template<typename T>
> void
> gt_ggc_mx (generic_wide_int <T> *)
