Commit Message

Hi Richard,
Thanks for the review.
On 07/10/16 20:11, Richard Biener wrote:
> On Fri, Oct 7, 2016 at 12:00 AM, kugan
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi,
>>
>> In vrp intersect_ranges, Richard recently changed it to create integer value
>> ranges when it is integer singleton.
>>
>> Maybe we should do the same when the other range is a complex ranges with
>> SSA_NAME (like [x+2, +INF])?
>>
>> Attached patch tries to do this. There are cases where it will be beneficial
>> as the testcase in the patch. (For this testcase to work with Early VRP, we
>> need the patch posted at
>> https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00413.html)
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>> regressions.
>
> This is not clearly a win, in fact it can completely lose an ASSERT_EXPR
> because there is no way to add its effect back as an equivalence. The
> current choice of always using the "left" keeps the ASSERT_EXPR range
> and is able to record the other range via an equivalence.
How about changing the order in Early VRP when we are dealing with the
same SSA_NAME in inner and outer scope. Here is a patch that does this.
Is this OK if no new regressions?
Thanks,
Kugan
> My thought on this was that we need to separate "ranges" and associated
> SSA names so we can introduce new ranges w/o the need for an SSA name
> (and thus we can create an equivalence to the ASSERT_EXPR range).
> IIRC I started on this at some point but never finished it ...
>
> Richard.
>
>> Thanks,
>> Kugan
>>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-10-07 Kugan Vivekanandarajah <kuganv@linaro.org>
>>
>> * gcc.dg/tree-ssa/evrp6.c: New test.
>>
>> gcc/ChangeLog:
>>
>> 2016-10-07 Kugan Vivekanandarajah <kuganv@linaro.org>
>>
>> * tree-vrp.c (intersect_ranges): If we failed to handle
>> the intersection and the other range involves computation with
>> symbolic values, choose integer range if available.
>>
>>
>>

Comments

Hi Richard,
On 10/10/16 20:13, Richard Biener wrote:
> On Sat, Oct 8, 2016 at 9:38 PM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi Richard,
>>
>> Thanks for the review.
>> On 07/10/16 20:11, Richard Biener wrote:
>>>
>>> On Fri, Oct 7, 2016 at 12:00 AM, kugan
>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> In vrp intersect_ranges, Richard recently changed it to create integer
>>>> value
>>>> ranges when it is integer singleton.
>>>>
>>>> Maybe we should do the same when the other range is a complex ranges with
>>>> SSA_NAME (like [x+2, +INF])?
>>>>
>>>> Attached patch tries to do this. There are cases where it will be
>>>> beneficial
>>>> as the testcase in the patch. (For this testcase to work with Early VRP,
>>>> we
>>>> need the patch posted at
>>>> https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00413.html)
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>>> regressions.
>>>
>>>
>>> This is not clearly a win, in fact it can completely lose an ASSERT_EXPR
>>> because there is no way to add its effect back as an equivalence. The
>>> current choice of always using the "left" keeps the ASSERT_EXPR range
>>> and is able to record the other range via an equivalence.
>>
>>
>> How about changing the order in Early VRP when we are dealing with the same
>> SSA_NAME in inner and outer scope. Here is a patch that does this. Is this
>> OK if no new regressions?
>
> I'm not sure if this is a good way forward. The failure with the testcase is
> that we don't extract a range for k from if (j < k) which I believe another
> patch from you addresses?
Yes, I have committed that. I am trying to add test cases for this and
thats when I stumbled on this:
For:
foo (int k, int j)
{
<bb 2>:
if (j_1(D) > 9)
goto <bb 3>;
else
goto <bb 6>;
<bb 3>:
if (j_1(D) < k_2(D))
goto <bb 4>;
else
goto <bb 6>;
<bb 4>:
k_3 = k_2(D) + 1;
if (k_2(D) <= 8)
goto <bb 5>;
else
goto <bb 6>;
<bb 5>:
abort ();
<bb 6>:
return j_1(D);
}
Before we look at - if (j_1(D) < k_2(D))
j_1 (D) has [10, +INF] EQUIVALENCES: { j_1(D) } (1 elements)
When we look at if (j_1(D) < k_2(D))
The range is [-INF, k_2(D) + -1] EQUIVALENCES: { j_1(D) } (1 elements)
We intersect:
[-INF, k_2(D) + -1] EQUIVALENCES: { j_1(D) } (1 elements)
and
[10, +INF] EQUIVALENCES: { j_1(D) } (1 elements)
to
[-INF, k_2(D) + -1] EQUIVALENCES: { j_1(D) } (1 elements)
Due to this, in if (j_1(D) < k_2(D)) , we get pessimistic value range
for k_2(D)
Thanks,
Kugan
> As said the issue is with the equivalence / value-range representation so
> you can't do sth like
>
> /* Discover VR when condition is true. */
> extract_range_for_var_from_comparison_expr (op0, code, op0, op1, &vr);
> if (old_vr->type == VR_RANGE || old_vr->type == VR_ANTI_RANGE)
> vrp_intersect_ranges (&vr, old_vr);
>
> /* If we found any usable VR, set the VR to ssa_name and create a
> PUSH old value in the stack with the old VR. */
> if (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE)
> {
> new_vr = vrp_value_range_pool.allocate ();
> *new_vr = vr;
> push_value_range (op0, new_vr);
> ->>> add equivalence to old_vr for new_vr.
>
> because old_vr and new_vr are the 'same' (they are associated with SSA name op0)
>
> Richard.
>
>> Thanks,
>> Kugan
>>
>>
>>
>>
>>
>>> My thought on this was that we need to separate "ranges" and associated
>>> SSA names so we can introduce new ranges w/o the need for an SSA name
>>> (and thus we can create an equivalence to the ASSERT_EXPR range).
>>> IIRC I started on this at some point but never finished it ...
>>>
>>> Richard.
>>>
>>>> Thanks,
>>>> Kugan
>>>>
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2016-10-07 Kugan Vivekanandarajah <kuganv@linaro.org>
>>>>
>>>> * gcc.dg/tree-ssa/evrp6.c: New test.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2016-10-07 Kugan Vivekanandarajah <kuganv@linaro.org>
>>>>
>>>> * tree-vrp.c (intersect_ranges): If we failed to handle
>>>> the intersection and the other range involves computation with
>>>> symbolic values, choose integer range if available.
>>>>
>>>>
>>>>
>>