| Message ID | 20170214071813.GR1849@tucnak |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 14, 2017 at 08:18:13AM +0100, Jakub Jelinek wrote: > On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: > > > 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. > > We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. > The only user of LIKELY_ADJUST is: > > if (res.knownrange) > res.range.likely = res.range.max; > else > { > // -- Here we know res.knownrage is false > res.range.likely = res.range.min; > if (likely_adjust && maybebase && base != 10) > // -- and here is the only user of likely_adjust > { > 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; > } > } Another argument I had was that if maybebase and base != 10, then if the range does not include zero (if it ever could be !res.knownrange in that case), then res.range.min won't be 1 and for base 16 won't be even 2, because all the values in the range will include the 0 or 0x prefixes. The only controversion then would be if the range was [0, 0], then bumping res.range.likely would not be appropriate. But such range is really res.knownrange and never anything else. Jakub
On 02/14/2017 12:18 AM, Jakub Jelinek wrote: > On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: >>> 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. > > We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. > The only user of LIKELY_ADJUST is: > > if (res.knownrange) > res.range.likely = res.range.max; > else > { > // -- Here we know res.knownrage is false > res.range.likely = res.range.min; > if (likely_adjust && maybebase && base != 10) > // -- and here is the only user of likely_adjust > { > 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; > } > } > >>> 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? > > That would then be (the only difference compared to the previous patch is > the last hunk) following. I can surely test that, I'm still convinced it > would work equally if that > (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > is just gcc_checking_assert. > > 2017-02-14 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust > variable, its initialization and use. > > --- 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,8 @@ format_integer (const directive &dir, tr > else > { > res.range.likely = res.range.min; > - if (likely_adjust && maybebase && base != 10) > + if (maybebase && base != 10 > + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) > { > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; You've removed all the comments that explain what's going on. If you must make the change (I see no justification for it now) please at least document it. Martin
On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote: > > @@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr > > else > > { > > res.range.likely = res.range.min; > > - if (likely_adjust && maybebase && base != 10) > > + if (maybebase && base != 10 > > + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) > > { > > if (res.range.min == 1) > > res.range.likely += base == 8 ? 1 : 2; > > You've removed all the comments that explain what's going on. If > you must make the change (I see no justification for it now) please > at least document it. I thought that is the: /* Add the adjustment for an argument whose range includes zero since it doesn't include the octal or hexadecimal base prefix. */ comment above the if. Jakub
On 02/14/2017 09:39 AM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote: >>> @@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr >>> else >>> { >>> res.range.likely = res.range.min; >>> - if (likely_adjust && maybebase && base != 10) >>> + if (maybebase && base != 10 >>> + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) >>> { >>> if (res.range.min == 1) >>> res.range.likely += base == 8 ? 1 : 2; >> >> You've removed all the comments that explain what's going on. If >> you must make the change (I see no justification for it now) please >> at least document it. > > I thought that is the: > /* Add the adjustment for an argument whose range includes zero > since it doesn't include the octal or hexadecimal base prefix. */ > comment above the if. That comment explains how the likely_adjust variable ("the adjustment") is being used, or more precisely, how it was being used in the first version of the patch. The comment became somewhat out of date with the committed version of the patch (this was my bad). The variable is documented where it's defined and again where it's assigned to. With the removal of those comments it seems especially important that the only remaining description of what's going on be accurate. The comment is outdated because it refers to "the adjustment" which doesn't exist anymore. (It was replaced by a flag in my commit). To bring it up to date it should say something like: /* Set the LIKELY counter to MIN. In base 8 and 16, when the argument is in range that includes zero, adjust it upward to include the length of the base prefix since in that case the MIN counter does include it. */ On a separate note, while testing the patch I noticed that it's not exactly equivalent to what's on trunk. Trunk silently accepts the call below but with the patch it complains. That's great (it should complain) but the change should be tested. More to my point, while in this case your change happened to fix a subtle bug (which I'm certainly happy about), it could have just as easily introduced one. char d[2]; void f (unsigned i) { if (i < 1234 || 12345 < i) i = 1234; __builtin_sprintf (d, "%#hhx", i); } Martin
On 02/14/2017 06:43 AM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 08:18:13AM +0100, Jakub Jelinek wrote: >> On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: >>>> 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. >> >> We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. >> The only user of LIKELY_ADJUST is: >> >> if (res.knownrange) >> res.range.likely = res.range.max; >> else >> { >> // -- Here we know res.knownrage is false >> res.range.likely = res.range.min; >> if (likely_adjust && maybebase && base != 10) >> // -- and here is the only user of likely_adjust >> { >> 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; >> } >> } Duh. You're obviously right. Still reading the rest of the thread. jeff
--- 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,8 @@ format_integer (const directive &dir, tr else { res.range.likely = res.range.min; - if (likely_adjust && maybebase && base != 10) + if (maybebase && base != 10 + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) { if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2;