Re: [patch,openacc] Fix PR71959: lto dump of callee counts

[Martin Jambor <mjambor@suse.cz>]

Hi,

I have noticed a few things...

On Thu, Sep 20 2018, Cesar Philippidis wrote:
> This is another old gomp4 patch that demotes an ICE in PR71959 to a
> linker warning. One problem here is that it is not clear if OpenACC
> allows individual member functions in C++ classes to be marked as acc
> routines. There's another issue accessing member data inside offloaded
> regions. We'll add some support for member data OpenACC 2.6, but some of
> the OpenACC C++ semantics are still unclear.
>
> Is this OK for trunk? I bootstrapped and regtested it for x86_64 Linux
> with nvptx offloading.
>
> Thanks,
> Cesar
> [PR71959] lto dump of callee counts
>
> 2018-XX-YY  Nathan Sidwell  <nathan@acm.org>
> 	    Cesar Philippidis  <cesar@codesourcery.com>
>
> 	gcc/
> 	* ipa-inline-analysis.c (inline_write_summary): Only dump callee
> 	counts when dumping the function's body.

...the changelog needs updating and....

>
> 	libgomp/
> 	* testsuite/libgomp.oacc-c++/pr71959.C: New.
> 	* testsuite/libgomp.oacc-c++/pr71959-a.C: New.
>
> (cherry picked from gomp-4_0-branch r239788)
> ---
>  gcc/ipa-fnsummary.c                           | 18 ++++++++---
>  .../testsuite/libgomp.oacc-c++/pr71959-a.C    | 31 +++++++++++++++++++
>  libgomp/testsuite/libgomp.oacc-c++/pr71959.C  | 31 +++++++++++++++++++
>  3 files changed, 75 insertions(+), 5 deletions(-)
>  create mode 100644 libgomp/testsuite/libgomp.oacc-c++/pr71959-a.C
>  create mode 100644 libgomp/testsuite/libgomp.oacc-c++/pr71959.C
>
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index 62095c6cf6f..e796b085e14 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -3409,8 +3409,10 @@ ipa_fn_summary_write (void)
>  	  int i;
>  	  size_time_entry *e;
>  	  struct condition *c;
> +	  int index = lto_symtab_encoder_encode (encoder, cnode);
> +	  bool body = encoder->nodes[index].body;
>  
> -	  streamer_write_uhwi (ob, lto_symtab_encoder_encode (encoder, cnode));
> +	  streamer_write_uhwi (ob, index);
>  	  streamer_write_hwi (ob, info->estimated_self_stack_size);
>  	  streamer_write_hwi (ob, info->self_size);
>  	  info->time.stream_out (ob);
> @@ -3453,10 +3455,16 @@ ipa_fn_summary_write (void)
>  	    info->array_index->stream_out (ob);
>  	  else
>  	    streamer_write_uhwi (ob, 0);
> -	  for (edge = cnode->callees; edge; edge = edge->next_callee)
> -	    write_ipa_call_summary (ob, edge);
> -	  for (edge = cnode->indirect_calls; edge; edge = edge->next_callee)
> -	    write_ipa_call_summary (ob, edge);
> +	  if (body)
> +	    {
> +	      /* Only write callee counts when we're emitting the
> +		 body, as the reader only knows about the callees when
> +		 the body's emitted.  */

this comment is wrong because write_ipa_call_summary does not only
stream counts but also sizes, predicates and other stuff.

I also wonder 1) whether the cnode->definition test that guards this
streaming should not be replaced by your body check (and why the
definition is not enough, really) and 2) why you don't have the same
problem with ipa_prop_write_jump_functions and the function it calls.

Martin

> +	      for (edge = cnode->callees; edge; edge = edge->next_callee)
> +		write_ipa_call_summary (ob, edge);
> +	      for (edge = cnode->indirect_calls; edge; edge = edge->next_callee)
> +		write_ipa_call_summary (ob, edge);
> +	    }
>  	}
>      }
>    streamer_write_char_stream (ob->main_stream, 0);