Re: [RFC][IPA-VRP] splits out the update_value_range calls from vrp_visit_stmt

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Aug 17, 2016 at 4:27 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi,
>
>
> On 16/08/16 20:58, Richard Biener wrote:
>>
>> On Tue, Aug 16, 2016 at 9:39 AM, kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>> Hi,
>>>
>>>> as said the refactoring that would be appreciated is to split out the
>>>> update_value_range calls
>>>> from the worker functions so you can call the respective functions
>>>> from the DOM implementations.
>>>> That they are globbed in vrp_visit_stmt currently is due to the API of
>>>> the SSA propagator.
>>>
>>>
>>>
>>> Here is a patch that just splits out the update_value_range calls
>>> visit_stmts. Bootstrapped and regression tested on x86_64-linux with no
>>> new
>>> regressions.
>>>
>>> I also verified few random fdump-tree-vrp1-details from stage2 to make
>>> sure
>>> they are same.
>>>
>>> Is this OK for trunk?
>>
>>
>> For vrp_visit_assignment_or_call please defer the question whether the
>> update
>> is interesting (for the internal call stuff) to the caller and always
>> return new_vr.
>>
>> Also do not perform the "not handled stmt" handling here but make the
>> return
>> value reflect whether we handled the stmt or not and put
>>
>>   /* Every other statement produces no useful ranges.  */
>>   FOR_EACH_SSA_TREE_OPERAND (def, stmt, iter, SSA_OP_DEF)
>>     set_value_range_to_varying (get_value_range (def));
>>
>> into the caller (as that's also a lattice-updating thing).
>>
>> +static enum ssa_prop_result
>> +vrp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p)
>> +{
>> +  value_range vr = VR_INITIALIZER;
>> +  tree lhs = gimple_get_lhs (stmt);
>> +  bool vr_found = vrp_visit_stmt_worker (stmt, taken_edge_p,
>> +                                        output_p, &vr);
>> +
>> +  if (lhs)
>> +    {
>> +      if (vr_found
>> +         && update_value_range (lhs, &vr))
>> +       {
>> +         *output_p = lhs;
>>
>> I think rather than computing LHS here you should use *output_p.
>>
>> Otherwise this looks good though I'd rename the _worker variants
>> to extract_range_from_phi_node, extract_range_from_stmt and
>> extract_range_from_assignment_or_call.
>>
>
> Please find the patch attached which addresses the comments above. Bootstrap
> and regression testing is ongoing. Is this of if there is no new
> regressions?

Yes, this looks good to me.

Thanks,
Richard.

> Thanks,
> Kugan
>
>
> gcc/ChangeLog:
>
> 2016-08-17  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         * tree-vrp.c (vrp_visit_assignment_or_call): Changed to Return VR.
>         (vrp_visit_cond_stmt): Just sets TAKEN_EDGE_P.
>         (vrp_visit_switch_stmt): Likewise.
>         (extract_range_from_stmt): Factored out from vrp_visit_stmt.
>         (extract_range_from_phi_node): Factored out from vrp_visit_phi_stmt.
>         (vrp_visit_stmt): Use extract_range_from_stmt.
>         (vrp_visit_phi_node): Use extract_range_from_phi_node.
>