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 <ja...@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
