On Mon, Aug 03, 2020 at 06:01:40PM +0200, Tobias Burnus wrote:
> On 7/20/20 11:20 PM, Tobias Burnus wrote:
> > On 7/20/20 9:12 PM, Jakub Jelinek wrote:
> > > I don't like this global variable.
> > > Can you please instead stick it into struct nesting_info and make
> > > sure it is
> > > cleared where it is allocated?
> >
> > Done. The existing code uses
> > struct nesting_info *info = XCNEW (struct nesting_info);
> > in create_nesting_tree; hence, the clearing is already done.
Sorry for the delay, wanted to look at it in more detail and didn't get to
it until now.
I think the patch as posted isn't the best thing to do, one thing is that
it will create the clauses even when OpenMP isn't enabled or the current
location isn't nested in an OpenMP region, or even when it is, but it isn't
inside of the clauses that contain gimple sequences.
I don't understand why convert_local_* has been changed at all.
Now, I guess one could get around that by having the
convert_nonlocal_*clauses* function set the pointer to address of a local
variable when it will deal with this and keep NULL otherwise and only add
clauses in those cases.
But it seems better to me to follow what we do in all the other cases.
E.g. if the outer function's PARM_DECL is referenced inside of OMP_PARALLEL
body, then it works fine because OMP_PARALLEL handling does:
save_local_var_chain = info->new_local_var_chain;
info->new_local_var_chain = NULL;
before the body walk and
if (info->new_local_var_chain)
declare_vars (info->new_local_var_chain,
gimple_seq_first_stmt (gimple_omp_body (stmt)),
false);
info->new_local_var_chain = save_local_var_chain;
afterwards, which means we don't really need any extra clauses, because the
temporaries will be declared inside of the parallel body rather than at some
outer scope.
I think we want to follow the suit with all the walks for clauses that
contain gimple sequences, so around the
OMP_CLAUSE_REDUCTION_GIMPLE_INIT
walk, around the
OMP_CLAUSE_REDUCTION_GIMPLE_MERGE
walk, around
OMP_CLAUSE_LASTPRIVATE_GIMPLE_SEQ
and
OMP_CLAUSE_LINEAR_GIMPLE_SEQ.
And probably both in the convert_nonlocal* case and in the convert_local*
case too.
Can you try that? Or do you want me to try?
Jakub
