On Fri, Oct 7, 2016 at 2:53 AM, kugan <kugan.vivekanandara...@linaro.org> wrote:
> Hi Richard,
>
> Thanks for the review.
>
>
> On 09/08/16 18:58, Richard Biener wrote:
>>
>> On Tue, Aug 9, 2016 at 12:58 AM, kugan
>> <kugan.vivekanandara...@linaro.org> wrote:
>>>
>>> Hi Jakub,
>>>
>>> Thanks for the review.
>>>
>>> On 08/08/16 16:40, Jakub Jelinek wrote:
>>>>
>>>>
>>>> On Mon, Aug 08, 2016 at 01:36:51PM +1000, kugan wrote:
>>>>>
>>>>>
>>>>> diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
>>>>> index c81b1a1..6e34433 100644
>>>>> --- a/gcc/tree-ssanames.h
>>>>> +++ b/gcc/tree-ssanames.h
>>>>> @@ -43,6 +43,9 @@ struct GTY(()) ptr_info_def
>>>>> above alignment. Access only through the same helper functions
>>>>> as
>>>>> align
>>>>> above. */
>>>>> unsigned int misalign;
>>>>> + /* When this pointer is knonw to be nnonnull this would be true
>>>>> otherwise
>>>>> + false. */
>>>>> + bool nonnull_p;
>>>>> };
>>>>
>>>>
>>>>
>>>> Why do you need this? Doesn't the pt.null bit represent that already?
>>>
>>>
>>>
>>> It looks like I can use this. As in gcc/tree-ssa-alias.h:
>>>
>>> /* Nonzero if the points-to set includes 'nothing', the points-to set
>>> includes memory at address NULL. */
>>> unsigned int null : 1;
>>>
>>> But in gcc/tree-ssa-alias.c, ptrs_compare_unequal has the following
>>> comment
>>> which says:
>>>
>>> /* ??? We'd like to handle ptr1 != NULL and ptr1 != ptr2
>>> but those require pt.null to be conservatively correct. */
>>>
>>> Does that means it can be wrong at times? I haven't looked it in detail
>>> yet
>>> but if it is, it would be a problem.
>>
>>
>> Yes, this bit is not correctly computed for all cases (well, it works
>> for trivial
>> ones but for example misses weaks and maybe some other corner-cases).
>>
>> Currently there are no consumers of this bit so the incorrectness
>> doesn't matter.
>>
>> I suggest you simply use that bit but set it conservatively from PTA (to
>> true)
>> for now and adjust it from VRP only.
>>
>> I do have some patches for fixinig some of the .nonnull_p issues in PTA
>> but
>> they come at a cost of more constraints.
>
>
> Here is a version of the patch that does this. I have a follow up patch to
> use this in ipa-vrp. I will send it for review once this is OK.
>
> Bootstrapped and regression tested on x86_64-linux-gnu with no new
> regressions. Is this OK for trunk?
@@ -6359,7 +6361,7 @@ find_what_var_points_to (tree fndecl, varinfo_t orig_vi)
if (vi->is_artificial_var)
{
if (vi->id == nothing_id)
- pt->null = 1;
+ ;
please leave this here.
pt_solution_singleton_p (struct pt_solution *pt, unsigned *uid)
{
if (pt->anything || pt->nonlocal || pt->escaped || pt->ipa_escaped
- || pt->null || pt->vars == NULL
+ || pt->vars == NULL
|| !bitmap_single_bit_set_p (pt->vars))
return false;
humm... this is a correctness problem I guess. A latent one currently
depending on who relies on it.
There is a single use of the above predicate which looks for the
points-to solution of a __builtin_alloca call. We should somehow
arrange for its solution to not include ->null. Or, simpler, change
the predicates name to pt_solution_singleton_or_null_p () as
the use does not care for pt->null.
+void
+set_ptr_nonnull (tree name)
+{
+ gcc_assert (POINTER_TYPE_P (TREE_TYPE (name)));
+ struct ptr_info_def *pi = get_ptr_info (name);
+ pi->pt.null = 0;
= false;
+}
There is the question on how pt.null semantics should be
with respect to pt.anything, pt.escaped and pt.nonlocal.
I think get_ptr_nonnull needs to return false if any
of those are set.
Richard.
>
> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2016-10-07 Kugan Vivekanandarajah <kug...@linaro.org>
>
> * tree-ssa-alias.c (dump_points_to_solution): Dump non null as it
> is set to null conservatively.
> * tree-ssa-structalias.c (find_what_var_points_to): Set to null
> conservatively.
> (pt_solution_singleton_p): Dont check null.
> * tree-ssanames.c (set_ptr_nonnull): New.
> (get_ptr_nonnull): Likewise.
> * tree-vrp.c (vrp_finalize): Set ptr that are nonnull.
> (evrp_dom_walker::before_dom_children): Likewise.
>
>>
>> Richard.
>>
>>>> Also, formatting and spelling:
>>>> s/knonw/known/
>>>> s/nnon/non/
>>>> s/bool /bool /
>>>
>>>
>>>
>>> I will change this.
>>>
>>> Thanks,
>>> Kugan
>>>>
>>>>
>>>>
>>>> Jakub
>>>>
>>>
>
