Re: [PATCH] tree-optimization/109304 - properly handle instrumented aliases

[Richard Biener <rguenther@suse.de>]

On Tue, 4 Apr 2023, Jan Hubicka wrote:

> > On Tue, 28 Mar 2023, Richard Biener wrote:
> > 
> > > When adjusting calls to reflect instrumentation we failed to handle
> > > calls to aliases since they appear to have no body.  Instead resort
> > > to symtab node availability.  The patch also avoids touching
> > > internal function calls in a more obvious way (builtins might
> > > have a body available).
> > > 
> > > profiledbootstrap & regtest running on x86_64-unknown-linux-gnu.
> > > 
> > > Honza - does this look OK?
> > > 	PR tree-optimization/109304
> > > 	* tree-profile.cc (tree_profiling): Use symtab node
> > > 	availability to decide whether to skip adjusting calls.
> > > 	Do not adjust calls to internal functions.
> > > @@ -842,12 +842,15 @@ tree_profiling (void)
> > >  	    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > >  	      {
> > >  		gcall *call = dyn_cast <gcall *> (gsi_stmt (gsi));
> > > -		if (!call)
> > > +		if (!call || gimple_call_internal_p (call))
> > >  		  continue;
> > >  
> > >  		/* We do not clear pure/const on decls without body.  */
> > >  		tree fndecl = gimple_call_fndecl (call);
> > > -		if (fndecl && !gimple_has_body_p (fndecl))
> > > +		cgraph_node *callee;
> > > +		if (fndecl
> > > +		    && (callee = cgraph_node::get (fndecl))
> > > +		    && callee->get_availability (node) == AVAIL_NOT_AVAILABLE)
> 
> As discussed earlier, the testcase I posted can be adjusted to put the
> const declared wrapper into another translation unit, so I think we will
> need to drop the visibility check completely.  But as discussed, it is
> wrong code issue, but not a regression, so we may go with the
> availability check as you suggest. So the patch is OK. 
> 
> 
> I wonder if we do not want to drop it everywhere (as we plan for next
> stage1 anyway).  I think similar ICE as in the PR can be produced with
> LTO. In normal situation declaration merging will do the right thing:
> If you have unit A calling const foo externally, it won't get processed
> by the code above.  However unit B declaring foo will get it downgraded
> to non-const.
> 
> Now at WPA time we will read both A and B and in declaration merging B's
> definition will prevail.  This won't happen if lto_symtab_merge_p
> returns false which can probably be triggered by adding warning/error
> attribute to B's declaration but not to A's.
> 
> It is however really side case and I am worried about dropping
> pure/const from builtin declarations...

Yeah, that's what I'm worried about as well.  I guess we'd need to
do the demotion to non-const/pure at WPA time and have a flag
in the cgraph node indicating instrument_add_{read,write}?  But
then how should we deal with C++ comdats instrumented in one TU
but not in another?  Does this mean we should do instrumentation
at IPA time instead of as small IPA pass before IPA?

That said, when there's a definition of say strlen in a TU and
that's instrumented we do want to drop pure from calls but if
not then we shouldn't worry.

Without LTO we'd still run into coverage issues but at least
with LTO we shouldn't ICE?

Richard.