On Sat, Oct 8, 2016 at 9:38 PM, kugan <kugan.vivekanandara...@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.vivekanandara...@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?
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 <kug...@linaro.org>
>>>
>>> * gcc.dg/tree-ssa/evrp6.c: New test.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-10-07 Kugan Vivekanandarajah <kug...@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.
>>>
>>>
>>>
>
