On Wed, Apr 23, 2014 at 3:29 PM, Richard Sandiford
<rdsandif...@googlemail.com> wrote:
> 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.
Ah, that makes sense now ;)
Thus the patch is ok.
Thanks,
Richard.
> 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> *)
