[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
--- Comment #16 from Aldy Hernandez aldyh at gcc dot gnu.org 2013-03-05
18:00:12 UTC ---
Created attachment 29590
-- http://gcc.gnu.org/bugzilla/attachment.cgi?id=29590
WIP: proposed patch special casing constant phi arguments
Ian.
Sure, I can handle SSA_NAME_VAR equality, but then we won't be able to handle
self referential operations like the one in gcc.dg/Wstrict-overflow-12.c. For
example, with your suggested approach (in this attached patch), we fail here:
for (i = 1, bits = 1; i 0; i += i) /* { dg-warning assuming signed
overflow does not occur correct warning } */
++bits;
Because we encounter something like this which is perfectly valid with your
approach:
i_1 = PHI 1(2), i_4(3)
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
--- Comment #15 from rguenther at suse dot de rguenther at suse dot de
2013-03-04 09:57:40 UTC ---
On Fri, 1 Mar 2013, aldyh at redhat dot com wrote:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
--- Comment #12 from Aldy Hernandez aldyh at redhat dot com 2013-03-01
19:17:32 UTC ---
--- Comment #11 from Ian Lance Taylor ian at airs dot com 2013-03-01
14:52:53 UTC ---
I suspect we can handle this case by observing that all the incoming values
to
the PHI node are constants.
Ian.
They're not all constants. In this particular case we have phis like this:
state_2 = PHI 0(6), state_3(12), 2(5)
I suppose we could chase the SSA def chain and if all the phi arguments
are either constants, or known to point to an SSA that has been
previously analyzed as constant, then we can avoid generating an
overflow. But we'd have to keep track of states across calls to
vrp_visit_phi_node. Is this what you had in mind, or am I
misunderstanding something?
Obviously, Richi's idea is much simpler :). With his suggestion we
could probably do with:
@@ -8111,11 +8109,9 @@ vrp_visit_phi_node (gimple phi)
if (cmp_max 0 || cmp_max 0)
{
if (!needs_overflow_infinity (TREE_TYPE (vr_result.max))
- || !vrp_var_may_overflow (lhs, phi))
+ || !vrp_var_may_overflow (lhs, phi)
+ || supports_overflow_infinity (TREE_TYPE (vr_result.max)))
vr_result.max = TYPE_MAX_VALUE (TREE_TYPE (vr_result.max));
- else if (supports_overflow_infinity (TREE_TYPE (vr_result.max)))
- vr_result.max =
- positive_overflow_infinity (TREE_TYPE (vr_result.max));
}
And similarly for MIN. In the above, supports_overflow_infinity is just
there to make sure we have a CONSTANT_CLASS_P. If that's not needed, we
could even do:
if (cmp_max 0 || cmp_max 0)
vr_result.max = TYPE_MAX_VALUE (TREE_TYPE (vr_result.max));
and be done with it.
That's what I was suggesting.
As for strict-overflow warnings in VRP my suggestion was that we
want to warn only when a folding result _changes_. Thus, propagate
-fwrapv and -fno-wrapv in parallel using two lattices and compare
the final result. That's way less prone to false positives.
Richard.
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
Richard Biener rguenth at gcc dot gnu.org changed:
What|Removed |Added
CC||iant at google dot com
--- Comment #10 from Richard Biener rguenth at gcc dot gnu.org 2013-03-01
11:45:54 UTC ---
(In reply to comment #9)
(In reply to comment #3)
We hit:
163724rguenth /* Similarly, if the new maximum is smaller or
larger
than
163724rguenththe previous one, go all the way to +INF. */
163724rguenth if (cmp_max 0 || cmp_max 0)
163724rguenth {
163724rguenth if (!needs_overflow_infinity (TREE_TYPE
(vr_result.max))
163724rguenth || !vrp_var_may_overflow (lhs, phi))
163724rguenth vr_result.max = TYPE_MAX_VALUE (TREE_TYPE
(vr_result.max));
163724rguenth else if (supports_overflow_infinity (TREE_TYPE
(vr_result.max)))
163724rguenth vr_result.max =
163724rguenth positive_overflow_infinity (TREE_TYPE
(vr_result.max));
163724rguenth }
(In reply to comment #8)
Does this seem like an approach worth exploring (this silences the
warning), or
does anyone have a better suggestion?
Isn't the problem that vrp_var_may_overflow returns true even though 'state'
cannot overflow? Jakub says:
As the IV (i) might overflow, vrp_var_may_overflow returns true.
In particular, chrec is SCEV_NOT_KNOWN. Thus it just in case sets
vr_result.max to +INF(OVF) and later on we warn about it.
Before hitting this code, vr_result contains the right range [0, 2], but it
doesn't know it can safely use that.
Couldn't be possible to detect this by the fact that 'state' does not depend
on
anything variable?
Also, in such a case, the algorithm cannot iterate more than the number of phi
nodes in the loop (if I understand the VRP correctly, which I most likely
don't).
But I looked around and I honestly don't know how to implement this idea.
In any case, your patch would need to adjust the code for the minimum also,
no?
Because the same behaviour can be triggered just by using negative numbers to
trigger a negative overflow infinity.
I think we should simply not set overflow on this code path in any case. We
have (OVF) just for the sake of -Wstrict-overflow warnings which in VRP are
implemeted in the worst possible way.
The issue with iterating is that you either can re-create the issue
with a different testcase or you blow up compile-time.
When somebody wants to improve VRP for SSA cycles then the way to go is
to compute the SSA SCC, classify it (in this case we only have assigns
from constants, not increments or decrements) and choose a solving
strathegy.
Oh, and of course strict-overflow warnings are all Ians fault ;)
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234 Ian Lance Taylor ian at airs dot com changed: What|Removed |Added CC||ian at airs dot com --- Comment #11 from Ian Lance Taylor ian at airs dot com 2013-03-01 14:52:53 UTC --- I suspect we can handle this case by observing that all the incoming values to the PHI node are constants. It's true that strict overflow warnings are my fault but I believe it was a necessary move to keep people from turning on -fwrapv.
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
--- Comment #12 from Aldy Hernandez aldyh at redhat dot com 2013-03-01
19:17:32 UTC ---
--- Comment #11 from Ian Lance Taylor ian at airs dot com 2013-03-01
14:52:53 UTC ---
I suspect we can handle this case by observing that all the incoming values to
the PHI node are constants.
Ian.
They're not all constants. In this particular case we have phis like this:
state_2 = PHI 0(6), state_3(12), 2(5)
I suppose we could chase the SSA def chain and if all the phi arguments
are either constants, or known to point to an SSA that has been
previously analyzed as constant, then we can avoid generating an
overflow. But we'd have to keep track of states across calls to
vrp_visit_phi_node. Is this what you had in mind, or am I
misunderstanding something?
Obviously, Richi's idea is much simpler :). With his suggestion we
could probably do with:
@@ -8111,11 +8109,9 @@ vrp_visit_phi_node (gimple phi)
if (cmp_max 0 || cmp_max 0)
{
if (!needs_overflow_infinity (TREE_TYPE (vr_result.max))
- || !vrp_var_may_overflow (lhs, phi))
+ || !vrp_var_may_overflow (lhs, phi)
+ || supports_overflow_infinity (TREE_TYPE (vr_result.max)))
vr_result.max = TYPE_MAX_VALUE (TREE_TYPE (vr_result.max));
- else if (supports_overflow_infinity (TREE_TYPE (vr_result.max)))
- vr_result.max =
- positive_overflow_infinity (TREE_TYPE (vr_result.max));
}
And similarly for MIN. In the above, supports_overflow_infinity is just
there to make sure we have a CONSTANT_CLASS_P. If that's not needed, we
could even do:
if (cmp_max 0 || cmp_max 0)
vr_result.max = TYPE_MAX_VALUE (TREE_TYPE (vr_result.max));
and be done with it.
Let me know. I am willing to entertain any approach y'all suggest.
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
--- Comment #13 from Ian Lance Taylor ian at airs dot com 2013-03-01 19:23:00
UTC ---
How hard would it be to test whether the values are all constant or have the
same SSA_NAME_VAR as the value we are setting?
My only concern about richi's suggestion is that it will miss some simple
cases.
for (i = 1; i = 0; i *= 2)
{
int j = fn1 () ? i : 1;
if (j = 0)
fn ();
}
Here i overflows and so does j, but I think your patch will teach VRP that j
does not overflow, so we will get no warning for j = 0.
Or something like that.
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234 --- Comment #14 from Aldy Hernandez aldyh at redhat dot com 2013-03-01 19:33:47 UTC --- On 03/01/13 13:23, ian at airs dot com wrote: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234 --- Comment #13 from Ian Lance Taylor ian at airs dot com 2013-03-01 19:23:00 UTC --- How hard would it be to test whether the values are all constant or have the same SSA_NAME_VAR as the value we are setting? That was actually my first idea, but it seemed too simple :). I will do so and report back. Thanks.
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
Aldy Hernandez aldyh at gcc dot gnu.org changed:
What|Removed |Added
CC||dnovillo at gcc dot
||gnu.org, rguenth at gcc dot
||gnu.org
--- Comment #8 from Aldy Hernandez aldyh at gcc dot gnu.org 2013-02-28
16:00:26 UTC ---
As Jakub explained in comment 3, this is a problem in the VRP code.
To avoid bouncing from max to max (or slowly increasing to +INF), we
immediately go to +INF(OVF), even if we have correct ranges.
One alternative that I show in the attached patch (comment 7: proof of concept
hack, untested), is to keep track of how many times we are iterating through an
SSA. If we go past an arbitrary number (say 10), we can then go to +INF.
So basically, keep track of a few states, but if it starts getting ridiculous
step it up to INF. Similarly for the cmp_min code. The relevant part of my
patch is shown below.
Does this seem like an approach worth exploring (this silences the warning), or
does anyone have a better suggestion?
- /* Similarly, if the new maximum is smaller or larger than
- the previous one, go all the way to +INF. */
- if (cmp_max 0 || cmp_max 0)
+ /* Adjust to a new maximum if it has changed. For the first few
+ iterations, adjust the maximum accordingly. However, if
+ we're iterating too much, go all the way to +INF to avoid
+ either bouncing around or iterating millions of times to
+ reach +INF. */
+ if ((cmp_max 0 || cmp_max 0))
{
- if (!needs_overflow_infinity (TREE_TYPE (vr_result.max))
- || !vrp_var_may_overflow (lhs, phi))
-vr_result.max = TYPE_MAX_VALUE (TREE_TYPE (vr_result.max));
- else if (supports_overflow_infinity (TREE_TYPE (vr_result.max)))
-vr_result.max =
-positive_overflow_infinity (TREE_TYPE (vr_result.max));
+ lhs_vr-visited++;
+ if (lhs_vr-visited 10) // Handle the first 10 iterations.
+vr_result.max = lhs_vr-max;
+ else
+{
+ if (!needs_overflow_infinity (TREE_TYPE (vr_result.max))
+ || !vrp_var_may_overflow (lhs, phi))
+vr_result.max = TYPE_MAX_VALUE (TREE_TYPE (vr_result.max));
+ else if (supports_overflow_infinity (TREE_TYPE (vr_result.max)))
+vr_result.max =
+ positive_overflow_infinity (TREE_TYPE (vr_result.max));
+}
}
[Bug tree-optimization/49234] [4.5/4.6/4.7/4.8 Regression] -Wstrict-overflow gives obviously unwarranted warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49234
Manuel López-Ibáñez manu at gcc dot gnu.org changed:
What|Removed |Added
CC||manu at gcc dot gnu.org
--- Comment #9 from Manuel López-Ibáñez manu at gcc dot gnu.org 2013-02-28
20:40:55 UTC ---
(In reply to comment #3)
We hit:
163724rguenth /* Similarly, if the new maximum is smaller or larger
than
163724rguenththe previous one, go all the way to +INF. */
163724rguenth if (cmp_max 0 || cmp_max 0)
163724rguenth {
163724rguenth if (!needs_overflow_infinity (TREE_TYPE
(vr_result.max))
163724rguenth || !vrp_var_may_overflow (lhs, phi))
163724rguenth vr_result.max = TYPE_MAX_VALUE (TREE_TYPE
(vr_result.max));
163724rguenth else if (supports_overflow_infinity (TREE_TYPE
(vr_result.max)))
163724rguenth vr_result.max =
163724rguenth positive_overflow_infinity (TREE_TYPE
(vr_result.max));
163724rguenth }
(In reply to comment #8)
Does this seem like an approach worth exploring (this silences the warning),
or
does anyone have a better suggestion?
Isn't the problem that vrp_var_may_overflow returns true even though 'state'
cannot overflow? Jakub says:
As the IV (i) might overflow, vrp_var_may_overflow returns true.
In particular, chrec is SCEV_NOT_KNOWN. Thus it just in case sets
vr_result.max to +INF(OVF) and later on we warn about it.
Before hitting this code, vr_result contains the right range [0, 2], but it
doesn't know it can safely use that.
Couldn't be possible to detect this by the fact that 'state' does not depend on
anything variable?
Also, in such a case, the algorithm cannot iterate more than the number of phi
nodes in the loop (if I understand the VRP correctly, which I most likely
don't).
But I looked around and I honestly don't know how to implement this idea.
In any case, your patch would need to adjust the code for the minimum also, no?
Because the same behaviour can be triggered just by using negative numbers to
trigger a negative overflow infinity.
