| Message ID | 20170807175138.3825d88e@pinnacle.lan |
|---|---|
| State | New |
| Headers | show |
Ping. On Mon, 7 Aug 2017 17:51:38 -0700 Kevin Buettner <kevinb@redhat.com> wrote: > On Wed, 10 May 2017 17:24:27 +0200 > Jakub Jelinek <jakub@redhat.com> wrote: > > > 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? > > I've worked out a way to do the DECL_CONTEXT and the scope change > together. The helper function should be usable for other constructs, > though I have not tested this yet. > > > 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? > > To the best of my knowledge, the debugger doesn't care about the > order. I've changed the code to prepend the FUNCTION_DECL to > BLOCK_VARS instead. > > How does this new version (below) look? > > I've done a "make bootstrap" and "make -k check". No regressions found > for x86_64. > > gcc/ChangeLog: > > * omp-expand.c (adjust_context_scope): New function. > (expand_parallel_call): Call adjust_context_scope. > --- > gcc/omp-expand.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c > index d6755cd..9eb0a89 100644 > --- a/gcc/omp-expand.c > +++ b/gcc/omp-expand.c > @@ -498,6 +498,42 @@ parallel_needs_hsa_kernel_p (struct omp_region *region) > return false; > } > > +/* Change DECL_CONTEXT of CHILD_FNDECL to that of the parent function. > + Add CHILD_FNDECL to decl chain of the supercontext of the block > + ENTRY_BLOCK - this is the block which originally contained the > + code from which CHILD_FNDECL was created. > + > + Together, these actions ensure that the debug info for the outlined > + function will be emitted with the correct lexical scope. */ > + > +static void > +adjust_context_and_scope (tree entry_block, tree child_fndecl) > +{ > + if (entry_block != NULL_TREE && TREE_CODE (entry_block) == BLOCK) > + { > + tree b = BLOCK_SUPERCONTEXT (entry_block); > + > + if (TREE_CODE (b) == BLOCK) > + { > + tree parent_fndecl; > + > + /* Follow supercontext chain until the parent fndecl > + is found. */ > + for (parent_fndecl = BLOCK_SUPERCONTEXT (b); > + TREE_CODE (parent_fndecl) == BLOCK; > + parent_fndecl = BLOCK_SUPERCONTEXT (parent_fndecl)) > + ; > + > + gcc_assert (TREE_CODE (parent_fndecl) == FUNCTION_DECL); > + > + DECL_CONTEXT (child_fndecl) = parent_fndecl; > + > + DECL_CHAIN (child_fndecl) = BLOCK_VARS (b); > + BLOCK_VARS (b) = child_fndecl; > + } > + } > +} > + > /* Build the function calls to GOMP_parallel_start etc to actually > generate the parallel operation. REGION is the parallel region > being expanded. BB is the block where to insert the code. WS_ARGS > @@ -667,6 +703,8 @@ expand_parallel_call (struct omp_region *region, basic_block bb, > tree child_fndecl = gimple_omp_parallel_child_fn (entry_stmt); > t2 = build_fold_addr_expr (child_fndecl); > > + adjust_context_and_scope (gimple_block (entry_stmt), child_fndecl); > + > vec_alloc (args, 4 + vec_safe_length (ws_args)); > args->quick_push (t2); > args->quick_push (t1);
On Thu, Sep 21, 2017 at 01:08:01PM -0700, Kevin Buettner wrote: > Ping. Ok, thanks. > On Mon, 7 Aug 2017 17:51:38 -0700 > Kevin Buettner <kevinb@redhat.com> wrote: > > > On Wed, 10 May 2017 17:24:27 +0200 > > Jakub Jelinek <jakub@redhat.com> wrote: > > > > > 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? > > > > I've worked out a way to do the DECL_CONTEXT and the scope change > > together. The helper function should be usable for other constructs, > > though I have not tested this yet. > > > > > 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? > > > > To the best of my knowledge, the debugger doesn't care about the > > order. I've changed the code to prepend the FUNCTION_DECL to > > BLOCK_VARS instead. > > > > How does this new version (below) look? > > > > I've done a "make bootstrap" and "make -k check". No regressions found > > for x86_64. > > > > gcc/ChangeLog: > > > > * omp-expand.c (adjust_context_scope): New function. > > (expand_parallel_call): Call adjust_context_scope. > > --- > > gcc/omp-expand.c | 38 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c > > index d6755cd..9eb0a89 100644 > > --- a/gcc/omp-expand.c > > +++ b/gcc/omp-expand.c > > @@ -498,6 +498,42 @@ parallel_needs_hsa_kernel_p (struct omp_region *region) > > return false; > > } > > > > +/* Change DECL_CONTEXT of CHILD_FNDECL to that of the parent function. > > + Add CHILD_FNDECL to decl chain of the supercontext of the block > > + ENTRY_BLOCK - this is the block which originally contained the > > + code from which CHILD_FNDECL was created. > > + > > + Together, these actions ensure that the debug info for the outlined > > + function will be emitted with the correct lexical scope. */ > > + > > +static void > > +adjust_context_and_scope (tree entry_block, tree child_fndecl) > > +{ > > + if (entry_block != NULL_TREE && TREE_CODE (entry_block) == BLOCK) > > + { > > + tree b = BLOCK_SUPERCONTEXT (entry_block); > > + > > + if (TREE_CODE (b) == BLOCK) > > + { > > + tree parent_fndecl; > > + > > + /* Follow supercontext chain until the parent fndecl > > + is found. */ > > + for (parent_fndecl = BLOCK_SUPERCONTEXT (b); > > + TREE_CODE (parent_fndecl) == BLOCK; > > + parent_fndecl = BLOCK_SUPERCONTEXT (parent_fndecl)) > > + ; > > + > > + gcc_assert (TREE_CODE (parent_fndecl) == FUNCTION_DECL); > > + > > + DECL_CONTEXT (child_fndecl) = parent_fndecl; > > + > > + DECL_CHAIN (child_fndecl) = BLOCK_VARS (b); > > + BLOCK_VARS (b) = child_fndecl; > > + } > > + } > > +} > > + > > /* Build the function calls to GOMP_parallel_start etc to actually > > generate the parallel operation. REGION is the parallel region > > being expanded. BB is the block where to insert the code. WS_ARGS > > @@ -667,6 +703,8 @@ expand_parallel_call (struct omp_region *region, basic_block bb, > > tree child_fndecl = gimple_omp_parallel_child_fn (entry_stmt); > > t2 = build_fold_addr_expr (child_fndecl); > > > > + adjust_context_and_scope (gimple_block (entry_stmt), child_fndecl); > > + > > vec_alloc (args, 4 + vec_safe_length (ws_args)); > > args->quick_push (t2); > > args->quick_push (t1); Jakub
On Fri, 22 Sep 2017 18:08:39 +0200 Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Sep 21, 2017 at 01:08:01PM -0700, Kevin Buettner wrote: > > Ping. > > Ok, thanks. Committed. Kevin > > On Mon, 7 Aug 2017 17:51:38 -0700 > > Kevin Buettner <kevinb@redhat.com> wrote: > > > > > On Wed, 10 May 2017 17:24:27 +0200 > > > Jakub Jelinek <jakub@redhat.com> wrote: > > > > > > > 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? > > > > > > I've worked out a way to do the DECL_CONTEXT and the scope change > > > together. The helper function should be usable for other constructs, > > > though I have not tested this yet. > > > > > > > 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? > > > > > > To the best of my knowledge, the debugger doesn't care about the > > > order. I've changed the code to prepend the FUNCTION_DECL to > > > BLOCK_VARS instead. > > > > > > How does this new version (below) look? > > > > > > I've done a "make bootstrap" and "make -k check". No regressions found > > > for x86_64. > > > > > > gcc/ChangeLog: > > > > > > * omp-expand.c (adjust_context_scope): New function. > > > (expand_parallel_call): Call adjust_context_scope. > > > --- > > > gcc/omp-expand.c | 38 ++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 38 insertions(+) > > > > > > diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c > > > index d6755cd..9eb0a89 100644 > > > --- a/gcc/omp-expand.c > > > +++ b/gcc/omp-expand.c > > > @@ -498,6 +498,42 @@ parallel_needs_hsa_kernel_p (struct omp_region *region) > > > return false; > > > } > > > > > > +/* Change DECL_CONTEXT of CHILD_FNDECL to that of the parent function. > > > + Add CHILD_FNDECL to decl chain of the supercontext of the block > > > + ENTRY_BLOCK - this is the block which originally contained the > > > + code from which CHILD_FNDECL was created. > > > + > > > + Together, these actions ensure that the debug info for the outlined > > > + function will be emitted with the correct lexical scope. */ > > > + > > > +static void > > > +adjust_context_and_scope (tree entry_block, tree child_fndecl) > > > +{ > > > + if (entry_block != NULL_TREE && TREE_CODE (entry_block) == BLOCK) > > > + { > > > + tree b = BLOCK_SUPERCONTEXT (entry_block); > > > + > > > + if (TREE_CODE (b) == BLOCK) > > > + { > > > + tree parent_fndecl; > > > + > > > + /* Follow supercontext chain until the parent fndecl > > > + is found. */ > > > + for (parent_fndecl = BLOCK_SUPERCONTEXT (b); > > > + TREE_CODE (parent_fndecl) == BLOCK; > > > + parent_fndecl = BLOCK_SUPERCONTEXT (parent_fndecl)) > > > + ; > > > + > > > + gcc_assert (TREE_CODE (parent_fndecl) == FUNCTION_DECL); > > > + > > > + DECL_CONTEXT (child_fndecl) = parent_fndecl; > > > + > > > + DECL_CHAIN (child_fndecl) = BLOCK_VARS (b); > > > + BLOCK_VARS (b) = child_fndecl; > > > + } > > > + } > > > +} > > > + > > > /* Build the function calls to GOMP_parallel_start etc to actually > > > generate the parallel operation. REGION is the parallel region > > > being expanded. BB is the block where to insert the code. WS_ARGS > > > @@ -667,6 +703,8 @@ expand_parallel_call (struct omp_region *region, basic_block bb, > > > tree child_fndecl = gimple_omp_parallel_child_fn (entry_stmt); > > > t2 = build_fold_addr_expr (child_fndecl); > > > > > > + adjust_context_and_scope (gimple_block (entry_stmt), child_fndecl); > > > + > > > vec_alloc (args, 4 + vec_safe_length (ws_args)); > > > args->quick_push (t2); > > > args->quick_push (t1); > > Jakub
diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c index d6755cd..9eb0a89 100644 --- a/gcc/omp-expand.c +++ b/gcc/omp-expand.c @@ -498,6 +498,42 @@ parallel_needs_hsa_kernel_p (struct omp_region *region) return false; } +/* Change DECL_CONTEXT of CHILD_FNDECL to that of the parent function. + Add CHILD_FNDECL to decl chain of the supercontext of the block + ENTRY_BLOCK - this is the block which originally contained the + code from which CHILD_FNDECL was created. + + Together, these actions ensure that the debug info for the outlined + function will be emitted with the correct lexical scope. */ + +static void +adjust_context_and_scope (tree entry_block, tree child_fndecl) +{ + if (entry_block != NULL_TREE && TREE_CODE (entry_block) == BLOCK) + { + tree b = BLOCK_SUPERCONTEXT (entry_block); + + if (TREE_CODE (b) == BLOCK) + { + tree parent_fndecl; + + /* Follow supercontext chain until the parent fndecl + is found. */ + for (parent_fndecl = BLOCK_SUPERCONTEXT (b); + TREE_CODE (parent_fndecl) == BLOCK; + parent_fndecl = BLOCK_SUPERCONTEXT (parent_fndecl)) + ; + + gcc_assert (TREE_CODE (parent_fndecl) == FUNCTION_DECL); + + DECL_CONTEXT (child_fndecl) = parent_fndecl; + + DECL_CHAIN (child_fndecl) = BLOCK_VARS (b); + BLOCK_VARS (b) = child_fndecl; + } + } +} + /* Build the function calls to GOMP_parallel_start etc to actually generate the parallel operation. REGION is the parallel region being expanded. BB is the block where to insert the code. WS_ARGS @@ -667,6 +703,8 @@ expand_parallel_call (struct omp_region *region, basic_block bb, tree child_fndecl = gimple_omp_parallel_child_fn (entry_stmt); t2 = build_fold_addr_expr (child_fndecl); + adjust_context_and_scope (gimple_block (entry_stmt), child_fndecl); + vec_alloc (args, 4 + vec_safe_length (ws_args)); args->quick_push (t2); args->quick_push (t1);