diff mbox

[committed,obvious] Remove invalid assert in find_func_aliases_for_builtin_call

Message ID 56693F6D.4040307@mentor.com
State New
Headers show

Commit Message

Tom de Vries Dec. 10, 2015, 9:01 a.m. UTC 
[ was : Re: [PATCH, PR68716] Fix GOMP/GOACC_parallel handling in 
find_func_clobbers ]

On 09/12/15 11:01, Tom de Vries wrote:
> [ 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.

And for that same reason, I've removed the same assert from 
find_func_aliases_for_builtin_call.

Committed to trunk as obvious.

Thanks,
- Tom
diff mbox

Patch

Remove invalid assert in find_func_aliases_for_builtin_call

2015-12-10  Tom de Vries  <tom@codesourcery.com>

	* tree-ssa-structalias.c (find_func_aliases_for_builtin_call): Remove
	invalid assert.

---
 gcc/tree-ssa-structalias.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 15e351e..c350862 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -4533,7 +4533,6 @@  find_func_aliases_for_builtin_call (struct function *fn, gcall *t)
 	      gcc_assert (TREE_CODE (fnarg) == ADDR_EXPR);
 	      tree fndecl = TREE_OPERAND (fnarg, 0);
 	      tree arg = gimple_call_arg (t, argpos);
-	      gcc_assert (TREE_CODE (arg) == ADDR_EXPR);
 
 	      varinfo_t fi = get_vi_for_tree (fndecl);
 	      find_func_aliases_for_call_arg (fi, 0, arg);