[PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/04/2017 01:07 AM, Jakub Jelinek wrote:
> On Fri, Feb 03, 2017 at 05:39:21PM +0100, Jakub Jelinek wrote:
>> Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html
>> patch, you would with my patch need just the tree_digits part,
>> and then the very last hunk in gimple-ssa-sprintf.c (with
>> likely_adjust &&
>> removed).  Because you do the adjustments only if !res.knownrange
>> and in that case you know argmin/argmax are actually dirtype's min/max,
>> so 0 must be in the range.
>
> You've committed the patch unnecessarily complicated, see above.
> The following gives the same testsuite result.
As you know, just getting the same testsuite result is not sufficient ;-)

>
> dirtype is one of the standard {un,}signed {char,short,int,long,long long}
> types, all of them have 0 in their ranges.
> For VR_RANGE we almost always set res.knownrange to true:
>           /* Set KNOWNRANGE if the argument is in a known subrange
>              of the directive's type (KNOWNRANGE may be reset below).  */
>           res.knownrange
>             = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
>                || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
> (the exception is in case that range clearly has to include zero),
> and reset it only if adjust_range_for_overflow returned true, which means
> it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
> includes zero.
> So IMNSHO likely_adjust in what you've committed is always true
> when you use it and thus just a useless computation and something to make
> the code harder to understand.
If KNOWNRANGE is false, then LIKELY_ADJUST will be true.  But I don't 
see how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true.


>
> Even if you don't trust this, with the ranges in argmin/argmax, it is
> IMHO undesirable to set it differently at the different code paths,
> if you want to check whether the final range includes zero and at least
> one another value, just do
> -      if (likely_adjust && maybebase && base != 10)
> +      if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
> 	   && maybebase && base != 10)
> Though, it is useless both for the above reason and for the reason that you
> actually do something only:
I'm not convinced it's useless, but it does seem advisable to bring test 
down to where it's actually used and to bse it strictly on 
argmin/argmax.  Can you test a patch which does that?


Jeff