On Wed, May 10, 2017 at 5:24 PM, Jakub Jelinek <ja...@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 <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.
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