This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)
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