Set nonnull attribute to ptr_info_def based on VRP

Hi Richard,

On 07/10/16 21:03, Richard Biener wrote:
> On Fri, Oct 7, 2016 at 2:53 AM, kugan <kugan.vivekanandarajah@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.vivekanandarajah@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.
Done.

>
>  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.

I have changed the name.

>
> +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.

I have added pt.anything.

I am assuming if pt.escaped, it cannot become null?
My intention is to eliminate for pt.nonlocal too. Like in:

static __attribute__((noinline, noclone))
int foo (int *p)
{
   if (!p)
     return 0;
   *p = 1;
}

struct st
{
   int a;
   int b;
};

int bar (struct st *s)
{

   if (!s)
     return 0;
   foo (&s->a);
   foo (&s->b);
}

And also in find_what_p_points_to we can overwrite pt.null. So I have 
changed that too.

Please find the updated patch attached. Does this look better?

Thanks,
Kugan

> Richard.
>
>>
>> Thanks,
>> Kugan
>>
>> gcc/ChangeLog:
>>
>> 2016-10-07  Kugan Vivekanandarajah  <kuganv@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
>>>>>
>>>>
>>

Set nonnull attribute to ptr_info_def based on VRP

Commit Message

Comments

Patch