On Fri, May 05, 2017 at 10:23:59AM -0700, Kevin Buettner wrote:
> On Fri, 5 May 2017 14:23:14 +0300 (MSK)
> Alexander Monakov <amona...@ispras.ru> wrote:
>
> > On Thu, 4 May 2017, Kevin Buettner wrote:
> > > diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c
> > > index 5c48b78..7029951 100644
> > > --- a/gcc/omp-expand.c
> > > +++ b/gcc/omp-expand.c
> > > @@ -667,6 +667,25 @@ expand_parallel_call (struct omp_region *region,
> > > basic_block bb,
> >
> > Outlined functions are also used for 'omp task' and 'omp target' regions,
> > but
> > here only 'omp parallel' is handled. Will this code need to be duplicated
> > for
> > those region types?
>
> For 'omp task' and 'omp target', I think it's possible or even likely
> that the original context which started these parallel tasks will no
> longer exist. So, it might not make sense to do something equivalent
> for 'task' and 'target'.
It depends. E.g. for #pragma omp taskloop without nogroup clause, it acts the
same as #pragma omp parallel in the nesting regard, the GOMP_taskloop* function
will
not return until all the tasks finished. Or if you have #pragma omp task and
#pragma omp taskwait on the next line, or #pragma omp taskgroup
around it, or #pragma omp target without nowait clause, it will behave the same.
Then there are cases where the encountering function will still be around,
but already not all the lexical scopes (or inline functions), e.g. if there
is #pragma omp taskwait or taskgroup etc. outside of the innermost lexical
scope(s), but still somewhere in the function. What the debugger should do
in that case is that it should figure out that the spot the task has been
created in has passed, so not show vars in the lexical scopes already left,
but still show others? Then of course if there is nothing waiting for the
task or async target in the current function, the function's frame could be
left, perhaps multiple callers too.
> > > tree child_fndecl = gimple_omp_parallel_child_fn (entry_stmt);
> > > t2 = build_fold_addr_expr (child_fndecl);
> > >
> > > + if (gimple_block (entry_stmt) != NULL_TREE
> > > + && TREE_CODE (gimple_block (entry_stmt)) == BLOCK)
> >
> > Here and also below, ...
> >
> > > + {
> > > + tree b = BLOCK_SUPERCONTEXT (gimple_block (entry_stmt));
> > > +
> > > + /* Add child_fndecl to var chain of the supercontext of the
> > > + block corresponding to entry_stmt. This ensures that debug
> > > + info for the outlined function will be emitted for the correct
> > > + lexical scope. */
> > > + if (b != NULL_TREE && TREE_CODE (b) == BLOCK)
> >
> > ... here, I'm curious why the conditionals are necessary -- I don't see why
> > the
> > conditions can be sometimes true and sometimes false. Sorry if I'm missing
> > something obvious.
gimple_block can be NULL. And, most calls of gimple_block that want to
ensure it is a BLOCK actually do verify it is a BLOCK, while it is unlikely
and it is usually just LTO that screws things up, I'd keep it.
> > > + if (b != NULL_TREE && TREE_CODE (b) == BLOCK)
>
> I check to make sure that b is a block so that I can later refer to
> BLOCK_VARS (b).
I believe BLOCK_SUPERCONTEXT of a BLOCK should always be non-NULL, either
another BLOCK, or FUNCTION_DECL. Thus I think b != NULL_TREE && is
redundant here.
What I don't like is that the patch is inconsistent, it sets DECL_CONTEXT
of the child function for all kinds of outlined functions, but then you just
choose one of the many places and add it into the BLOCK tree. Any reason
why the DECL_CONTEXT change can't be done in a helper function together
with all the changes you've added into omp-expand.c, and then call it from
expand_omp_parallel (with the child_fn and entry_stmt arguments) so that
you can call it easily also for other constructs, either now or later on?
Also, is there any rationale on appending the FUNCTION_DECL to BLOCK_VARS
instead of prepending it there (which is cheaper)? Does the debugger
care about relative order of those artificial functions vs. other
variables in the lexical scope?
Jakub
