| Message ID | 20170214203233.GA1849@tucnak |
|---|---|
| State | New |
| Headers | show |
On 02/14/2017 01:32 PM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote: >> 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. */ > > So for a comment, what about following then? With or without > the IMNSHO useless > && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) If the condition is redundant (it seems like it could be) it shouldn't be included in the patch. It seems like an opportunity for further simplification. I'm sure it's not the only one, either. >> 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. > > Yeah, indeed. That should be a clear argument for why writing it in > so many places is bad, it is simply much more error-prone, there are > too many cases to get right. No argument there. There's always room for improvement, cleanup, or refactoring. Martin > >> char d[2]; >> >> void f (unsigned i) >> { >> if (i < 1234 || 12345 < i) >> i = 1234; >> >> __builtin_sprintf (d, "%#hhx", i); >> } > > What happens is that because the original range doesn't contain zero > you set likely_adjust to false and then never update it again because > the implicit cast changed the range. > > If some version of the patch is approved, I'll leave addition of this > testcase to you (incrementally). > > 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-14 21:21:56.048745037 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-02-14 21:25:20.939033174 +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); > @@ -1364,14 +1347,19 @@ format_integer (const directive &dir, tr > res.range.max = MAX (max1, max2); > } > > - /* Add the adjustment for an argument whose range includes zero > - since it doesn't include the octal or hexadecimal base prefix. */ > + /* If the range is known, use the maximum as the likely length. */ > if (res.knownrange) > res.range.likely = res.range.max; > else > { > + /* Otherwise, use the minimum. Except for the case where for %#x or > + %#o the minimum is just for a single value in the range (0) and > + for all other values it is something longer, like 0x1 or 01. > + Use the length for value 1 in that case instead as the likely > + length. */ > 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; > > > Jakub >
On 02/14/2017 01:32 PM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote: >> 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. */ > > So for a comment, what about following then? With or without > the IMNSHO useless > && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > >> 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. > > Yeah, indeed. That should be a clear argument for why writing it in > so many places is bad, it is simply much more error-prone, there are > too many cases to get right. > >> char d[2]; >> >> void f (unsigned i) >> { >> if (i < 1234 || 12345 < i) >> i = 1234; >> >> __builtin_sprintf (d, "%#hhx", i); >> } > > What happens is that because the original range doesn't contain zero > you set likely_adjust to false and then never update it again because > the implicit cast changed the range. > > If some version of the patch is approved, I'll leave addition of this > testcase to you (incrementally). > > 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. This is fine. And the addition of the test from Martin is pre-approved as well. jeff
--- gcc/gimple-ssa-sprintf.c.jj 2017-02-14 21:21:56.048745037 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-14 21:25:20.939033174 +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); @@ -1364,14 +1347,19 @@ format_integer (const directive &dir, tr res.range.max = MAX (max1, max2); } - /* Add the adjustment for an argument whose range includes zero - since it doesn't include the octal or hexadecimal base prefix. */ + /* If the range is known, use the maximum as the likely length. */ if (res.knownrange) res.range.likely = res.range.max; else { + /* Otherwise, use the minimum. Except for the case where for %#x or + %#o the minimum is just for a single value in the range (0) and + for all other values it is something longer, like 0x1 or 01. + Use the length for value 1 in that case instead as the likely + length. */ 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;