Set nonnull attribute to ptr_info_def based on VRP
Richard Biener
richard.guenther@gmail.com
Wed Oct 12 12:20:00 GMT 2016
On Wed, Oct 12, 2016 at 8:46 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> 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?
Sure can it 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);
> }
I don't think this is sth PTA handles.
> 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?
@@ -515,8 +515,8 @@ dump_points_to_solution (FILE *file, struct pt_solution *pt)
if (pt->ipa_escaped)
fprintf (file, ", points-to unit escaped");
- if (pt->null)
- fprintf (file, ", points-to NULL");
+ if (!pt->null)
+ fprintf (file, ", points-to non NULL");
please keep the original dumping. !pt->null doesn't mean it points to
sth not NULL.
It might point to nothing. And as I said pt->nonlocal / pt->escaped
include NULL.
@@ -6391,9 +6393,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;
- else if (vi->id == escaped_id)
+ if (vi->id == escaped_id)
{
if (in_ipa_mode)
pt->ipa_escaped = 1;
please keep this as well.
@@ -6451,6 +6451,7 @@ find_what_p_points_to (tree fndecl, tree p)
struct ptr_info_def *pi;
tree lookup_p = p;
varinfo_t vi;
+ bool nonnull = get_ptr_nonnull (p);
/* For parameters, get at the points-to set for the actual parm
decl. */
@@ -6466,6 +6467,8 @@ find_what_p_points_to (tree fndecl, tree p)
pi = get_ptr_info (p);
pi->pt = find_what_var_points_to (fndecl, vi);
+ if (nonnull)
+ set_ptr_nonnull (p);
}
what does this try to do? Preserve info VRP computed across PTA?
I think we didn't yet sort out the nonlocal/escaped vs. null handling properly
(or how PTA should handle get_ptr_nonnull). The way you are using it
asks for pt.null to be orthogonal to nonlocal/escaped and thus having
nonlocal or escaped would also require setting ptr.null in PTA. It then
would be also more canonical to set it for pt.anything as well. Which
means conservatively handling it would be equivalent to flipping its
semantic and changing its name to pt.nonnull.
That said, you seem to be simply "reserving" the bit for VRP, keeping it
conservatively true when "not computed". I guess I'm fine with this for now
but it should be documented in the header file that way.
Richard.
> 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
>>>>>>
>>>>>
>>>
>
More information about the Gcc-patches
mailing list