Re: [PATCH] Fix ICE for static vars in offloaded functions

[Tom de Vries]

On 03/07/2018 04:01 PM, Richard Biener wrote:

On Wed, 7 Mar 2018, Tom de Vries wrote:


On 03/07/2018 02:29 PM, Richard Biener wrote:

On Wed, 7 Mar 2018, Jakub Jelinek wrote:


On Wed, Mar 07, 2018 at 02:20:26PM +0100, Tom de Vries wrote:

Fix ICE for static vars in offloaded functions

2018-03-06  Tom de Vries  

PR lto/84592
* varpool.c (varpool_node::get_create): Mark static variables in
offloaded functions as offloadable.

* testsuite/libgomp.c/pr84592-2.c: New test.
* testsuite/libgomp.c/pr84592.c: New test.
* testsuite/libgomp.oacc-c-c++-common/pr84592-3.c: New test.


Ok, thanks


+  bool in_offload_func
+   = (cfun
+  && TREE_STATIC (decl)
+  && (lookup_attribute ("omp target entr

I think you want to use decl_function_context (decl) here,
not rely on magic cfun being set.  The whole varpool.c file
doesn't mention cfun yet and you shoudln't either.



decl_function_context (decl) returns main:
...
(gdb) call debug_generic_expr (decl)
test
(gdb) call  decl_function_context (decl)
$2 = (tree_node *) 0x76978c00
(gdb) call debug_generic_expr ($2)
main
...
while the function annotated as being an offload function is main._omp_fn.0.


Well, that's because the static isn't duplicated (it can't be) so it
retains the original context.



[ Actually the static is duplicated in replace_by_duplicate_decl, but 
the statements using it are not rewritten to use the duplicate, so 
indeed, effectively it's not duplicated. ]



The varpool_node::get_create is called during cgraph_edge::rebuild_edges here
in expand_omp_target:


But at this point it's not created but just looked up, right?



No, the varpool_node is created at that point.


I think the fix is to mark the decl as offloaded when we walk the IL
of the outlined function.  The current point looks like a hack.



OK, I'll try to find a better fix location.

Thanks,
- Tom


Richard.


...
7087  /* Fix the callgraph edges for child_cfun.  Those for cfun will
be
7088 fixed in a following pass.  */
7089  push_cfun (child_cfun);
7090  if (need_asm)
7091assign_assembler_name_if_needed (child_fn);
7092  cgraph_edge::rebuild_edges ();
...

Thanks,
- Tom

Re: [PATCH] Fix ICE for static vars in offloaded functions

[Jakub Jelinek]

On Wed, Mar 07, 2018 at 02:20:26PM +0100, Tom de Vries wrote:
> Fix ICE for static vars in offloaded functions
> 
> 2018-03-06  Tom de Vries  
> 
>   PR lto/84592
>   * varpool.c (varpool_node::get_create): Mark static variables in
>   offloaded functions as offloadable.
> 
>   * testsuite/libgomp.c/pr84592-2.c: New test.
>   * testsuite/libgomp.c/pr84592.c: New test.
>   * testsuite/libgomp.oacc-c-c++-common/pr84592-3.c: New test.

Ok, thanks

Jakub

Re: [PATCH] Fix ICE for static vars in offloaded functions

[Richard Biener]

On Wed, 7 Mar 2018, Tom de Vries wrote:

> On 03/07/2018 02:29 PM, Richard Biener wrote:
> > On Wed, 7 Mar 2018, Jakub Jelinek wrote:
> > 
> > > On Wed, Mar 07, 2018 at 02:20:26PM +0100, Tom de Vries wrote:
> > > > Fix ICE for static vars in offloaded functions
> > > > 
> > > > 2018-03-06  Tom de Vries  
> > > > 
> > > > PR lto/84592
> > > > * varpool.c (varpool_node::get_create): Mark static variables in
> > > > offloaded functions as offloadable.
> > > > 
> > > > * testsuite/libgomp.c/pr84592-2.c: New test.
> > > > * testsuite/libgomp.c/pr84592.c: New test.
> > > > * testsuite/libgomp.oacc-c-c++-common/pr84592-3.c: New test.
> > > 
> > > Ok, thanks
> > 
> > +  bool in_offload_func
> > +   = (cfun
> > +  && TREE_STATIC (decl)
> > +  && (lookup_attribute ("omp target entr
> > 
> > I think you want to use decl_function_context (decl) here,
> > not rely on magic cfun being set.  The whole varpool.c file
> > doesn't mention cfun yet and you shoudln't either.
> > 
> 
> decl_function_context (decl) returns main:
> ...
> (gdb) call debug_generic_expr (decl)
> test
> (gdb) call  decl_function_context (decl)
> $2 = (tree_node *) 0x76978c00
> (gdb) call debug_generic_expr ($2)
> main
> ...
> while the function annotated as being an offload function is main._omp_fn.0.

Well, that's because the static isn't duplicated (it can't be) so it 
retains the original context.

> The varpool_node::get_create is called during cgraph_edge::rebuild_edges here
> in expand_omp_target:

But at this point it's not created but just looked up, right?

I think the fix is to mark the decl as offloaded when we walk the IL
of the outlined function.  The current point looks like a hack.

Richard.

> ...
> 7087  /* Fix the callgraph edges for child_cfun.  Those for cfun will
> be
> 7088 fixed in a following pass.  */
> 7089  push_cfun (child_cfun);
> 7090  if (need_asm)
> 7091assign_assembler_name_if_needed (child_fn);
> 7092  cgraph_edge::rebuild_edges ();
> ...
> 
> Thanks,
> - Tom
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

Re: [PATCH] Fix ICE for static vars in offloaded functions

[Tom de Vries]

On 03/07/2018 02:29 PM, Richard Biener wrote:

On Wed, 7 Mar 2018, Jakub Jelinek wrote:


On Wed, Mar 07, 2018 at 02:20:26PM +0100, Tom de Vries wrote:

Fix ICE for static vars in offloaded functions

2018-03-06  Tom de Vries  

PR lto/84592
* varpool.c (varpool_node::get_create): Mark static variables in
offloaded functions as offloadable.

* testsuite/libgomp.c/pr84592-2.c: New test.
* testsuite/libgomp.c/pr84592.c: New test.
* testsuite/libgomp.oacc-c-c++-common/pr84592-3.c: New test.


Ok, thanks


+  bool in_offload_func
+   = (cfun
+  && TREE_STATIC (decl)
+  && (lookup_attribute ("omp target entr

I think you want to use decl_function_context (decl) here,
not rely on magic cfun being set.  The whole varpool.c file
doesn't mention cfun yet and you shoudln't either.



decl_function_context (decl) returns main:
...
(gdb) call debug_generic_expr (decl)
test
(gdb) call  decl_function_context (decl)
$2 = (tree_node *) 0x76978c00
(gdb) call debug_generic_expr ($2)
main
...
while the function annotated as being an offload function is main._omp_fn.0.

The varpool_node::get_create is called during cgraph_edge::rebuild_edges 
here in expand_omp_target:

...
7087  /* Fix the callgraph edges for child_cfun.  Those for cfun 
will be

7088 fixed in a following pass.  */
7089  push_cfun (child_cfun);
7090  if (need_asm)
7091assign_assembler_name_if_needed (child_fn);
7092  cgraph_edge::rebuild_edges ();
...

Thanks,
- Tom

Re: [PATCH] Fix ICE for static vars in offloaded functions

[Jakub Jelinek]

On Wed, Mar 07, 2018 at 02:29:48PM +0100, Richard Biener wrote:
> On Wed, 7 Mar 2018, Jakub Jelinek wrote:
> 
> > On Wed, Mar 07, 2018 at 02:20:26PM +0100, Tom de Vries wrote:
> > > Fix ICE for static vars in offloaded functions
> > > 
> > > 2018-03-06  Tom de Vries  
> > > 
> > >   PR lto/84592
> > >   * varpool.c (varpool_node::get_create): Mark static variables in
> > >   offloaded functions as offloadable.
> > > 
> > >   * testsuite/libgomp.c/pr84592-2.c: New test.
> > >   * testsuite/libgomp.c/pr84592.c: New test.
> > >   * testsuite/libgomp.oacc-c-c++-common/pr84592-3.c: New test.
> > 
> > Ok, thanks
> 
> +  bool in_offload_func
> +   = (cfun
> +  && TREE_STATIC (decl)
> +  && (lookup_attribute ("omp target entr
> 
> I think you want to use decl_function_context (decl) here, 
> not rely on magic cfun being set.  The whole varpool.c file
> doesn't mention cfun yet and you shoudln't either.

Oops, sure, thanks for catching it.

Jakub

Re: [PATCH] Fix ICE for static vars in offloaded functions

[Richard Biener]

On Wed, 7 Mar 2018, Jakub Jelinek wrote:

> On Wed, Mar 07, 2018 at 02:20:26PM +0100, Tom de Vries wrote:
> > Fix ICE for static vars in offloaded functions
> > 
> > 2018-03-06  Tom de Vries  
> > 
> > PR lto/84592
> > * varpool.c (varpool_node::get_create): Mark static variables in
> > offloaded functions as offloadable.
> > 
> > * testsuite/libgomp.c/pr84592-2.c: New test.
> > * testsuite/libgomp.c/pr84592.c: New test.
> > * testsuite/libgomp.oacc-c-c++-common/pr84592-3.c: New test.
> 
> Ok, thanks

+  bool in_offload_func
+   = (cfun
+  && TREE_STATIC (decl)
+  && (lookup_attribute ("omp target entr

I think you want to use decl_function_context (decl) here, 
not rely on magic cfun being set.  The whole varpool.c file
doesn't mention cfun yet and you shoudln't either.

please fix if you already committed the fix.

Thanks,
Richard.

>   Jakub
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)