This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Output DIEs for outlined OpenMP functions in correct lexical scope
On Wed, May 10, 2017 at 5:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 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 <amonakov@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.
gimple_block should always be either NULL or a BLOCK. It's *_CONTEXT that
can be types, decls or blocks.
>> > > + 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.
Yes.
>
> 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