On Sun, Nov 16, 2014 at 8:52 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On November 16, 2014 5:22:26 AM CET, Patrick Palka <patr...@parcs.ath.cx>
> wrote:
>>On Wed, Nov 12, 2014 at 3:38 AM, Richard Biener
>><richard.guent...@gmail.com> wrote:
>>> On Wed, Nov 12, 2014 at 5:17 AM, Patrick Palka <patr...@parcs.ath.cx>
>>wrote:
>>>> On Tue, Nov 11, 2014 at 8:48 AM, Richard Biener
>>>> <richard.guent...@gmail.com> wrote:
>>>>> On Tue, Nov 11, 2014 at 1:10 PM, Patrick Palka
>><patr...@parcs.ath.cx> wrote:
>>>>>> This patch is a replacement for the 2nd VRP refactoring patch. It
>>>>>> simply teaches VRP to look through widening type conversions when
>>>>>> finding suitable edge assertions, e.g.
>>>>>>
>>>>>> bool p = x != y;
>>>>>> int q = (int) p;
>>>>>> if (q == 0) // new edge assert: p == 0 and therefore x == y
>>>>>
>>>>> I think the proper fix is to forward x != y to q == 0 instead of
>>this one.
>>>>> That said - the tree-ssa-forwprop.c restriction on only forwarding
>>>>> single-uses into conditions is clearly bogus here. I suggest to
>>>>> relax it for conversions and compares. Like with
>>>>>
>>>>> Index: tree-ssa-forwprop.c
>>>>> ===================================================================
>>>>> --- tree-ssa-forwprop.c (revision 217349)
>>>>> +++ tree-ssa-forwprop.c (working copy)
>>>>> @@ -476,7 +476,7 @@ forward_propagate_into_comparison_1 (gim
>>>>> {
>>>>> rhs0 = rhs_to_tree (TREE_TYPE (op1), def_stmt);
>>>>> tmp = combine_cond_expr_cond (stmt, code, type,
>>>>> - rhs0, op1, !single_use0_p);
>>>>> + rhs0, op1, false);
>>>>> if (tmp)
>>>>> return tmp;
>>>>> }
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>
>>>> That makes sense. Attached is what I have so far. I relaxed the
>>>> forwprop restriction in the case of comparing an integer constant
>>with
>>>> a comparison or with a conversion from a boolean value. (If I allow
>>>> all conversions, not just those from a boolean value, then a couple
>>of
>>>> -Wstrict-overflow faillures trigger..) Does the change look
>>sensible?
>>>> Should the logic be duplicated for the case when TREE_CODE (op1) ==
>>>> SSA_NAME? Thanks for your help so far!
>>>
>>> It looks good though I'd have allowed all kinds of conversions, not
>>only
>>> those from booleans.
>>>
>>> If the patch tests ok with that change it is ok.
>>
>>Sadly changing the patch to propagate all kinds of conversions, not
>>only just those from booleans, introduces regressions that I don't
>>know how to adequately fix.
>
> OK. The original patch propagating only bool conversions is ok then. Can
> you list the failures you have seen when propagating more?
>
> Thanks,
> Richard.
>
gcc.dg/Wstrict-overflow-26.c: the patch introduces a bogus overflow
warning here. I was able to fix this one by not warning on equality
comparisons, but fixing it caused ...
gcc.dg/Wstrict-overflow-18.c: ... this to regress. I was able to this
one too, by teaching VRP to emit an overflow warning when simplifying
non-equality comparisons to equality comparisons (in this case i > 0
--> i != 0) when the operand has the range [0, +INF(OVF)].
g++.dg/calloc.C: this regression I wasn't able to fix. One problem is
that VRP is no longer able to simplify the "m * 4 > 0" comparison in
the following testcase:
void
f (int n)
{
size_t m = n;
if (m > (size_t)-1 / 4)
abort ();
if (n != 0) // used to be m != 0 before the patch
{
...
if (m * 4 > 0)
..
}
}
This happens because VRP has no way of knowing that if n != 0 then m
!= 0. I hacked up a fix for this deficiency in VRP by looking at an
operand's def stmts when adding edge assertions, so that for the
conditional "n != 0" we will also insert the edge assertion "m != 0".
But still calloc.C regressed, most notably in the slsr pass where the
pass was unable to combine two ssa names which had equivalent
definitions. At that point I gave up.
I also played around with folding "m > (size_t)-1 / 4" to "n < 0" in
the hopes that a subsequent pass would move the definition for m
closer to its use (assuming such a pass exists) so that m will see n's
ASSERT_EXPRs in m's def chain. But that didn't work too well because
apparently such a pass doesn't exist.
