Output DIEs for outlined OpenMP functions in correct lexical scope

Message ID 20170807175138.3825d88e@pinnacle.lan
State New
Headers show

Commit Message

Kevin Buettner Aug. 8, 2017, 12:51 a.m.
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(+)

Comments

Kevin Buettner Sept. 21, 2017, 8:08 p.m. | #1
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);
Jakub Jelinek Sept. 22, 2017, 4:08 p.m. | #2
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
Kevin Buettner Oct. 2, 2017, 12:29 a.m. | #3
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

Patch

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);