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

[Richard Biener <rguenther at suse dot de>]

On Mon, 30 Nov 2015, 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.
> 
> OK for stage3 trunk if bootstrap and reg-test succeeds?

-                        || node->address_taken);
+                        || (node->address_taken
+                            && !node->parallelized_function));

please add a comment here on why this is safe.

Ok with this change.

Thanks,
Richard.