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

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

[Richard Biener]

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?

Honza - ping!

> 	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.
> 
> 	* gcc.dg/pr109304.c: New testcase.
> ---
>  gcc/testsuite/gcc.dg/pr109304.c | 12 ++++++++++++
>  gcc/tree-profile.cc             |  9 ++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr109304.c
> 
> diff --git a/gcc/testsuite/gcc.dg/pr109304.c b/gcc/testsuite/gcc.dg/pr109304.c
> new file mode 100644
> index 00000000000..d435b04d4d5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr109304.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-profiling "-fprofile-generate" } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-options "-O3 -fprofile-generate -fPIC -fno-semantic-interposition" } */
> +
> +int PyUnicode_FindChar_i;
> +int PyUnicode_FindChar()
> +{
> +  while (PyUnicode_FindChar_i)
> +    if (PyUnicode_FindChar())
> +      break;
> +}
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index 6f9a43e4bd5..da300d5f9e8 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -808,7 +808,7 @@ tree_profiling (void)
>        {
>  	if (!gimple_has_body_p (node->decl)
>  	    || !(!node->clone_of
> -	    || node->decl != node->clone_of->decl))
> +		 || node->decl != node->clone_of->decl))
>  	  continue;
>  
>  	/* Don't profile functions produced for builtin stuff.  */
> @@ -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)
>  		  continue;
>  
>  		/* Drop the const attribute from the call type (the pure
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

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

[Jan Hubicka]

> 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...

Honza

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

[Jakub Jelinek]

On Tue, Apr 04, 2023 at 01:21:40AM +0200, Jan Hubicka via Gcc-patches wrote:
> It is however really side case and I am worried about dropping
> pure/const from builtin declarations...

Yeah, that can certainly break stuff.  See e.g. the recently fixed
ICE when memcmp wasn't pure in PR109258.

	Jakub

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

[Jan Hubicka]

> On Tue, Apr 04, 2023 at 01:21:40AM +0200, Jan Hubicka via Gcc-patches wrote:
> > It is however really side case and I am worried about dropping
> > pure/const from builtin declarations...
> 
> Yeah, that can certainly break stuff.  See e.g. the recently fixed
> ICE when memcmp wasn't pure in PR109258.

Yep, i think itis better to poke about this in stage1 (it is a can of
worms).  Clearly we have conflict here: if memcmp is implemented locally
one can construct a testcase where profile would be rejected on
-fprofile-use time if const flag is not cleared :(.  But it should be
rare thing happening in practice.

Honza
>
> 	Jakub
> 

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

[Richard Biener]

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.

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

[Richard Biener]

On Tue, 4 Apr 2023, Jan Hubicka wrote:

> > On Tue, Apr 04, 2023 at 01:21:40AM +0200, Jan Hubicka via Gcc-patches wrote:
> > > It is however really side case and I am worried about dropping
> > > pure/const from builtin declarations...
> > 
> > Yeah, that can certainly break stuff.  See e.g. the recently fixed
> > ICE when memcmp wasn't pure in PR109258.
> 
> Yep, i think itis better to poke about this in stage1 (it is a can of
> worms).  Clearly we have conflict here: if memcmp is implemented locally
> one can construct a testcase where profile would be rejected on
> -fprofile-use time if const flag is not cleared :(.  But it should be
> rare thing happening in practice.

If we have a locally implemented memcmp then calls to it shouldn't
be marked 'built-in' ...   But then when the compiler itself
creates a memcmp call it would need to resolve to a not instrumented
library copy, or alternatively we should tell the compiler it cannot
emit such a call (but for some builtins not being able to emit them
might prove interesting).

Richard.

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

[Jan Hubicka]

> 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?

I do not think LTO is of any help here.  You can allways call non-LTO
const function from outer-world and that function can will end up
calling back to instrumented const function in your unit which
effectively makes the extenral const function non-const.
> 
> 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?

I am not sure I see your point here...
We could avoid demoting builtins to avoid ICEs and have coverage
mismathces, but how LTO makes difference?

Honza
> 
> Richard.

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

[Richard Biener]

On Fri, 14 Apr 2023, Jan Hubicka wrote:

> > 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?
> 
> I do not think LTO is of any help here.  You can allways call non-LTO
> const function from outer-world and that function can will end up
> calling back to instrumented const function in your unit which
> effectively makes the extenral const function non-const.

Hmm, true.

> > 
> > 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?
> 
> I am not sure I see your point here...
> We could avoid demoting builtins to avoid ICEs and have coverage
> mismathces, but how LTO makes difference?

At least we get more functions local, but yes, we can still trigger
the issue.

So what's the solution?  All functions that are not leaf or possibly
instrumented have to be called as if they were not pure/const,
including builtins?  As we've said we're going to ICE quite a bit
when const/pure builtins suddenly are no longer const/pure.

Richard.

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

[Jan Hubicka]

> > 
> > I do not think LTO is of any help here.  You can allways call non-LTO
> > const function from outer-world and that function can will end up
> > calling back to instrumented const function in your unit which
> > effectively makes the extenral const function non-const.
> 
> Hmm, true.
> 
> > > 
> > > 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?
> > 
> > I am not sure I see your point here...
> > We could avoid demoting builtins to avoid ICEs and have coverage
> > mismathces, but how LTO makes difference?
> 
> At least we get more functions local, but yes, we can still trigger
> the issue.
> 
> So what's the solution?  All functions that are not leaf or possibly
> instrumented have to be called as if they were not pure/const,
> including builtins?  As we've said we're going to ICE quite a bit
> when const/pure builtins suddenly are no longer const/pure.

Yep, I can't think of any easier solution than handling all functions as
not pure/const as soon as something instrumented is ever inlined to a
given function.  For builtins this is fun indeed.  We can special case
those that are always expanded inline at least...

Honza
> 
> Richard.

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

[Richard Biener]

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.

	* gcc.dg/pr109304.c: New testcase.
---
 gcc/testsuite/gcc.dg/pr109304.c | 12 ++++++++++++
 gcc/tree-profile.cc             |  9 ++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr109304.c

diff --git a/gcc/testsuite/gcc.dg/pr109304.c b/gcc/testsuite/gcc.dg/pr109304.c
new file mode 100644
index 00000000000..d435b04d4d5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr109304.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-profiling "-fprofile-generate" } */
+/* { dg-require-effective-target fpic } */
+/* { dg-options "-O3 -fprofile-generate -fPIC -fno-semantic-interposition" } */
+
+int PyUnicode_FindChar_i;
+int PyUnicode_FindChar()
+{
+  while (PyUnicode_FindChar_i)
+    if (PyUnicode_FindChar())
+      break;
+}
diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index 6f9a43e4bd5..da300d5f9e8 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -808,7 +808,7 @@ tree_profiling (void)
       {
 	if (!gimple_has_body_p (node->decl)
 	    || !(!node->clone_of
-	    || node->decl != node->clone_of->decl))
+		 || node->decl != node->clone_of->decl))
 	  continue;
 
 	/* Don't profile functions produced for builtin stuff.  */
@@ -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)
 		  continue;
 
 		/* Drop the const attribute from the call type (the pure
-- 
2.35.3