fix latent bootstrap-debug issue (modref, tree-inline, tree jump-threading)

fix latent bootstrap-debug issue (modref, tree-inline, tree jump-threading)

[Alexandre Oliva]

I've hit a bootstrap-debug error involving large subprograms in
gcc/ada/sem_ch12.adb.  I'm afraid I couldn't narrow it down to a
reasonable testcase.

thread1 made different decisions about a block containing a
builtin_eh_filter call because in one compilation, estimate_num_insns
found a cgraph_node for the builtin and could thus get to the
is_simple_builtin test, but in the other it didn't.  With different
insn counts, one stage jump-threaded and the other didn't, and the
resulting code diverged quite a bit.

The reason the builtin had a cgraph_node in one case but not the other
was that modref got a chance to analyze the builtin call when it was
the first stmt in the block, and that created the cgraph_node.
However, when it was preceded by debug stmts, the loop in
analyze_function was cut short after the first debug stmt, because the
summary so far was not useful.

This patch fixes both issues: skip debug stmts in the analyze_function
loop, so as to prevent them from affecting any decisions in the loop,
and enable the insn count estimator to get to the is_simple_builtin
test when a cgraph_node has not been created for the builtin.

Regstrapped on x86_64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	* ipa-modref.c (analyze_function): Skip debug stmts.
	* tree-inline.c (estimate_num_insn): Consider builtins even
	without a cgraph_node.
---
 gcc/ipa-modref.c  |    3 ++-
 gcc/tree-inline.c |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
index fafd804d4bae4..f0cddbb077aaa 100644
--- a/gcc/ipa-modref.c
+++ b/gcc/ipa-modref.c
@@ -2108,7 +2108,8 @@ analyze_function (function *f, bool ipa)
   FOR_EACH_BB_FN (bb, f)
     {
       gimple_stmt_iterator si;
-      for (si = gsi_after_labels (bb); !gsi_end_p (si); gsi_next (&si))
+      for (si = gsi_start_nondebug_after_labels_bb (bb);
+	   !gsi_end_p (si); gsi_next_nondebug (&si))
 	{
 	  if (!analyze_stmt (summary, summary_lto,
 			     gsi_stmt (si), ipa, &recursive_calls)
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index d0e9f52d5f138..636130fe0019e 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -4436,8 +4436,8 @@ estimate_num_insns (gimple *stmt, eni_weights *weights)
 	    /* Do not special case builtins where we see the body.
 	       This just confuse inliner.  */
 	    struct cgraph_node *node;
-	    if (!(node = cgraph_node::get (decl))
-		|| node->definition)
+	    if ((node = cgraph_node::get (decl))
+		&& node->definition)
 	      ;
 	    /* For buitins that are likely expanded to nothing or
 	       inlined do not account operand costs.  */


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: fix latent bootstrap-debug issue (modref, tree-inline, tree jump-threading)

[Jan Hubicka]

> 
> for  gcc/ChangeLog
> 
> 	* ipa-modref.c (analyze_function): Skip debug stmts.
> 	* tree-inline.c (estimate_num_insn): Consider builtins even
> 	without a cgraph_node.

OK, thanks for looking into this issue!
(for mainline and release brances bit later)
> ---
>  gcc/ipa-modref.c  |    3 ++-
>  gcc/tree-inline.c |    4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
> index fafd804d4bae4..f0cddbb077aaa 100644
> --- a/gcc/ipa-modref.c
> +++ b/gcc/ipa-modref.c
> @@ -2108,7 +2108,8 @@ analyze_function (function *f, bool ipa)
>    FOR_EACH_BB_FN (bb, f)
>      {
>        gimple_stmt_iterator si;
> -      for (si = gsi_after_labels (bb); !gsi_end_p (si); gsi_next (&si))
> +      for (si = gsi_start_nondebug_after_labels_bb (bb);
> +	   !gsi_end_p (si); gsi_next_nondebug (&si))

It seems that analye_stmt indeed does not skip debug stmts.  It is very
odd we got so far without hitting build difference.

Honza
>  	{
>  	  if (!analyze_stmt (summary, summary_lto,
>  			     gsi_stmt (si), ipa, &recursive_calls)
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index d0e9f52d5f138..636130fe0019e 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -4436,8 +4436,8 @@ estimate_num_insns (gimple *stmt, eni_weights *weights)
>  	    /* Do not special case builtins where we see the body.
>  	       This just confuse inliner.  */
>  	    struct cgraph_node *node;
> -	    if (!(node = cgraph_node::get (decl))
> -		|| node->definition)
> +	    if ((node = cgraph_node::get (decl))
> +		&& node->definition)
>  	      ;
>  	    /* For buitins that are likely expanded to nothing or
>  	       inlined do not account operand costs.  */
> 
> 
> -- 
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: fix latent bootstrap-debug issue (modref, tree-inline, tree jump-threading)

[Alexandre Oliva]

On Aug 22, 2021, Jan Hubicka <hubicka@ucw.cz> wrote:

> OK, thanks for looking into this issue!

Thanks, I've finally installed it in the trunk.

> It seems that analye_stmt indeed does not skip debug stmts.  It is very
> odd we got so far without hitting build difference.

Indeed.  That got me thinking...  The comments state:

     If the statement cannot be analyzed (for any reason), the entire
     function cannot be analyzed by modref.

but the implementation also tests, for every statement:

	      || ((!summary || !summary->useful_p (ecf_flags, false))
		  && (!summary_lto
		      || !summary_lto->useful_p (ecf_flags, false))))

which means that, if the first stmt of a block doesn't add useful
information to the summary, we give up.  Was this really the intent?

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: fix latent bootstrap-debug issue (modref, tree-inline, tree jump-threading)

[Jan Hubicka]

> On Aug 22, 2021, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> > OK, thanks for looking into this issue!
> 
> Thanks, I've finally installed it in the trunk.
> 
> > It seems that analye_stmt indeed does not skip debug stmts.  It is very
> > odd we got so far without hitting build difference.
> 
> Indeed.  That got me thinking...  The comments state:
> 
>      If the statement cannot be analyzed (for any reason), the entire
>      function cannot be analyzed by modref.
> 
> but the implementation also tests, for every statement:
> 
> 	      || ((!summary || !summary->useful_p (ecf_flags, false))
> 		  && (!summary_lto
> 		      || !summary_lto->useful_p (ecf_flags, false))))
> 
> which means that, if the first stmt of a block doesn't add useful
> information to the summary, we give up.  Was this really the intent?
It is just early exit condition in case we already found enough side
effects to give up on any useful info about loads/stores.
Summaries are computed from optimistic one (function has no side
effects) by becoming more pesimistic as statements are being visited.

So in most cases useful_p is true at the begining of the loop (since we
see now loads/stores).  I suppose we was hitting the difference because
in const functions the summaries can be !useful_p from begining of the
loop.  I guess it is harmless to process the first statement in that
case (if we do not produce debug bootstrap difference)
Honza
> 
> -- 
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>