Re: [RFC][IPA-VRP] Re-factor tree-vrp to factor out common code

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Aug 17, 2016 at 4:50 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>
> On 17/08/16 08:20, kugan wrote:
>>
>> Hi,
>>
>> On 16/08/16 21:56, Richard Biener wrote:
>>>
>>> On Tue, Aug 16, 2016 at 10:09 AM, kugan
>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On 23/07/16 20:12, kugan wrote:
>>>>>
>>>>>
>>>>> Hi Richard,
>>>>>
>>>>>>> As we had value_range_type in tree-ssanames.h why not put value_range
>>>>>>> there?
>>>>>>>
>>>>>> For IPA_VRP, we now need value_range used in ipa-prop.h (in ipa-vrp
>>>>>> patch). Based on this, attached patch now adds struct value_range to
>>>>>> tree-ssanames.h and fixes the header files. Please note that I also
>>>>>> had
>>>>>> to add other headers in few places due to the dependency. Are you OK
>>>>>> with this ?
>>>>>
>>>>>
>>>>> Here is alternate patch where we keep struct value_range and enum
>>>>> value_range_type to tree-vrp.h. May be it is a better approach? Please
>>>>> let me know what is your preference.
>>>>>
>>>>
>>>> Ping?
>>>>
>>>> This patch places value_range_type and value_range in tree-vrp.h. May be
>>>> this is better?
>>>>
>>>> Alternate patch which keeps value_range_type and value_range in
>>>> tree-ssanames.h is in:
>>>> https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01491.html
>>>>
>>>> I also added the necessary header files changed needed for ipa-vrp as
>>>> part
>>>> of this patch so that changes needed are clear.
>>>
>>>
>>> I think tree-vrp.h is a better place.  Please don't export functions
>>> you don't need
>>> (the _1 helpers).
>
> I had unintentionally removed a static from a _1 helper. I think thats what
> caused the confusion. I also added constant modifiers to parameters mainly
> due to ipa-vrp passing second parameters to lattice operation as const.
>
>> Agreed.
>>
>> I have exported the following for now:
>> +extern void vrp_intersect_ranges (value_range *vr0, value_range *vr1);
>> +extern void vrp_meet (value_range *vr0, const value_range *vr1);
>> +extern void dump_value_range (FILE *, const value_range *);
>
>
> This again is the exported operations.
>
>> It might be useful to add vrp_unary_op, vrp_binary_op on value_range.
>> But that is for later if that is needed.
>>
>>>
>>> I still believe sharing vrp_initialize/finalize is wrong and the
>>> lattice setup / teardown
>>> should be split out.
>>
>>
>> I have done that too as part of the early-vrp patch in:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01155.html
>>
>> I just wanted to focus on the functionality required for the IPA-VRP on
>> this patch.
>
>
>
> Attaching the latest version of the patch. Is this OK?

Yes.

Thanks,
Richard.

> Thanks,
> Kugan