On Sat, Feb 04, 2017 at 09:07:23AM +0100, Jakub Jelinek wrote:
> You've committed the patch unnecessarily complicated, see above.
> The following gives the same testsuite result.
>
> 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.
>
> 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:
> if (res.range.min == 1)
> res.range.likely += base == 8 ? 1 : 2;
> else if (res.range.min == 2
> && base == 16
> && (dir.width[0] == 2 || dir.prec[0] == 2))
> ++res.range.likely;
> where if the range doesn't include zero, you would never get
> res.range.min of 1 and for base == 16 also not 2.
>
> 2017-02-04 Jakub Jelinek <[email protected]>
>
> PR tree-optimization/79327
> * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
> variable, its initialization and use.
Now bootstrapped/regtested on x86_64-linux and i686-linux (ignore the
testsuite/ChangeLog part, I've committed that part in another commit already),
ok for trunk?
> --- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.000000000 +0100
> +++ gcc/gimple-ssa-sprintf.c 2017-02-04 08:45:33.173709580 +0100
> @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr
> of the format string by returning [-1, -1]. */
> return fmtresult ();
>
> - /* True if the LIKELY counter should be adjusted upward from the MIN
> - counter to account for arguments with unknown values. */
> - bool likely_adjust = false;
> -
> fmtresult res;
>
> /* Using either the range the non-constant argument is in, or its
> @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr
>
> res.argmin = argmin;
> res.argmax = argmax;
> -
> - /* Set the adjustment for an argument whose range includes
> - zero since that doesn't include the octal or hexadecimal
> - base prefix. */
> - wide_int wzero = wi::zero (wi::get_precision (min));
> - if (wi::le_p (min, wzero, SIGNED)
> - && !wi::neg_p (max))
> - likely_adjust = true;
> }
> else if (range_type == VR_ANTI_RANGE)
> {
> @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr
>
> if (!argmin)
> {
> - /* Set the adjustment for an argument whose range includes
> - zero since that doesn't include the octal or hexadecimal
> - base prefix. */
> - likely_adjust = true;
> -
> if (TREE_CODE (argtype) == POINTER_TYPE)
> {
> argmin = build_int_cst (pointer_sized_int_node, 0);
> @@ -1371,7 +1354,7 @@ format_integer (const directive &dir, tr
> else
> {
> res.range.likely = res.range.min;
> - if (likely_adjust && maybebase && base != 10)
> + if (maybebase && base != 10)
> {
> if (res.range.min == 1)
> res.range.likely += base == 8 ? 1 : 2;
Jakub
