[PR68716] Fix GOMP/GOACC_parallel handling in find_func_clobbers

[ was: Re: [PATCH, PR46032] Handle BUILT_IN_GOMP_PARALLEL in ipa-pta ]

On 30/11/15 13:11, Tom de Vries wrote:
> On 30/11/15 10:16, Richard Biener wrote:
>> On Mon, 30 Nov 2015, Tom de Vries wrote:
>>
>>> Hi,
>>>
>>> this patch fixes PR46032.
>>>
>>> It handles a call:
>>> ...
>>>    __builtin_GOMP_parallel (fn, data, num_threads, flags)
>>> ...
>>> as:
>>> ...
>>>    fn (data)
>>> ...
>>> in ipa-pta.
>>>
>>> This improves ipa-pta alias analysis in the parallelized function fn,
>>> and
>>> allows vectorization in the testcase without a runtime alias test.
>>>
>>> Bootstrapped and reg-tested on x86_64.
>>>
>>> OK for stage3 trunk?
>>
>> +             /* Assign the passed argument to the appropriate incoming
>> +                parameter of the function.  */
>> +             struct constraint_expr lhs ;
>> +             lhs = get_function_part_constraint (fi, fi_parm_base + 0);
>> +             auto_vec<ce_s, 2> rhsc;
>> +             struct constraint_expr *rhsp;
>> +             get_constraint_for_rhs (arg, &rhsc);
>> +             while (rhsc.length () != 0)
>> +               {
>> +                 rhsp = &rhsc.last ();
>> +                 process_constraint (new_constraint (lhs, *rhsp));
>> +                 rhsc.pop ();
>> +               }
>>
>> please use style used elsewhere with
>>
>>               FOR_EACH_VEC_ELT (rhsc, j, rhsp)
>>                 process_constraint (new_constraint (lhs, *rhsp));
>>               rhsc.truncate (0);
>>
>
> That code was copied from find_func_aliases_for_call.
> I've factored out the bit that I copied as
> find_func_aliases_for_call_arg, and fixed the style there (and dropped
> 'rhsc.truncate (0)' since AFAIU it's redundant at the end of a function).
>
>> +             /* Parameter passed by value is used.  */
>> +             lhs = get_function_part_constraint (fi, fi_uses);
>> +             struct constraint_expr *rhsp;
>> +             get_constraint_for_address_of (arg, &rhsc);
>>
>> This isn't correct - you want to use get_constraint_for (arg, &rhsc).
>> After all rhs is already an ADDR_EXPR.
>>
>
> Can we add an assert somewhere to detect this incorrect usage?
>
>> +             FOR_EACH_VEC_ELT (rhsc, j, rhsp)
>> +               process_constraint (new_constraint (lhs, *rhsp));
>> +             rhsc.truncate (0);
>> +
>> +             /* The caller clobbers what the callee does.  */
>> +             lhs = get_function_part_constraint (fi, fi_clobbers);
>> +             rhs = get_function_part_constraint (cfi, fi_clobbers);
>> +             process_constraint (new_constraint (lhs, rhs));
>> +
>> +             /* The caller uses what the callee does.  */
>> +             lhs = get_function_part_constraint (fi, fi_uses);
>> +             rhs = get_function_part_constraint (cfi, fi_uses);
>> +             process_constraint (new_constraint (lhs, rhs));
>>
>> I don't see why you need those.  The solver should compute these
>> in even better precision (context sensitive on the call side).
>>
>> The same is true for the function parameter.  That is, the only
>> needed part of the patch should be that making sure we see
>> the "direct" call and assign parameters correctly.
>>
>
> Dropped this bit.

Dropping the find_func_clobbers bit (in particular, the part copying the 
clobbers) caused PR68716.

Basically the find_func_clobbers bit tries to emulate the generic 
handling of calls in find_func_clobbers, but pretends the call is
   fn (data)
when handling
   __builtin_GOMP_parallel (fn, data, num_threads, flags)
.

I used the same comments as in the generic call handling to make it 
clear what part of the generic call handling is emulated.

Compared to the originally posted patch, the changes are:
- removed 'gcc_assert (TREE_CODE (arg) == ADDR_EXPR)'. Ran into a case
   where arg is NULL.
- use get_constraint_for instead of get_constraint_for_address_of as
   requested in a review comment above.
- handle GOACC_parallel as well

Bootstrapped and reg-tested on x86_64.

OK for stage3 trunk?

Thanks,
- Tom

On Wed, Dec 09, 2015 at 11:01:31AM +0100, Tom de Vries wrote:
> 	PR tree-optimization/68716
> 	* tree-ssa-structalias.c (find_func_clobbers): Fix handling of
> 	BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOMP_PARALLEL.

Pasto in ChangeLog entry?

	Jakub

On Wed, 9 Dec 2015, Jakub Jelinek wrote:

> On Wed, Dec 09, 2015 at 11:01:31AM +0100, Tom de Vries wrote:
> > 	PR tree-optimization/68716
> > 	* tree-ssa-structalias.c (find_func_clobbers): Fix handling of
> > 	BUILT_IN_GOMP_PARALLEL and BUILT_IN_GOMP_PARALLEL.
> 
> Pasto in ChangeLog entry?

Ok with that fixed.

Richard.