[PATCH]: Fix PR26725: ICE in check_reg_live, at haifa-sched.c:4645

[Richard Sandiford]

Maxim Kuvyrkov <mkuvyrkov@ispras.ru> writes:
> Hi!
>
> This is patch, that resolves PR26725:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26725
>
> I have my doubts if this is the best way to fix the problem, so am 
> waiting for any comments.
>
> --
> Maxim
> 2006-03-17  Maxim Kuvyrkov <mkuvyrkov@ispras.ru>
>
> 	* cfgcleanup.c (try_forward_edges): Fix PR26725.
> 	* haifa-sched.c (check_reg_live): Change signature.
> 	* sched-int.h (check_reg_live): Ditto.
> 	* sched-rgn.c (check_reg_live): Update parameters.
> 	* sched-ebb.c (check_reg_live): Ditto.
> --- mainline/gcc/cfgcleanup.c	(revision 18741)
> +++ mainline/gcc/cfgcleanup.c	(local)
> @@ -541,6 +541,26 @@ try_forward_edges (int mode, basic_block
>  	  if (threaded && target != EXIT_BLOCK_PTR)
>  	    {
>  	      notice_new_block (redirect_edge_and_branch_force (e, target));
> +
> +              /* PR26725: Consider the case:
> +                 // Basic block B
> +                 if (p) goto C; // jump1
> +                 ...
> +
> +                 C: // Basic block C
> +                 if (p) goto D; // jump2
> +                 else
> +                 ...
> +
> +                 D: // Basic block D
> +                 ...
> +                 We redirected jump1 to basic block D.  Now some registers,
> +                 that were considered live at the end of block B due to that
> +                 they are alive at the 'else' branch of jump2, should be
> +                 removed from the global_live_at_end regset of basic block
> +                 B.  */                 
> +              b->flags |= BB_DIRTY;
> +
>  	      if (dump_file)
>  		fprintf (dump_file, "Conditionals threaded.\n");
>  	    }

I came across this independently while trying to bootstrap
on mips64-linux-gnu.  A different style of testcase is attached.

I don't think your patch is the right way to go though.  As you say,
the problem is simply that redirecting an edge A->B so that it goes
from A->C can change the register liveness information for A.
rtl_redirect_edge_and_branch already realises this and sets
BB_DIRTY appropriately.

I think the bug is in rtl_redirect_edge_and_branch_force, which does
_not_ detect this case if the initial redirect_edge_and_branch attempt
fails:

--------------------------------------------------------------------------
static basic_block
rtl_redirect_edge_and_branch_force (edge e, basic_block target)
{
  if (redirect_edge_and_branch (e, target)
      || e->dest == target)
    return NULL;

  /* In case the edge redirection failed, try to force it to be non-fallthru
     and redirect newly created simplejump.  */
  return force_nonfallthru_and_redirect (e, target);
}
--------------------------------------------------------------------------

I.e., force_nonfallthru_and_redirect does not set BB_DIRTY if E's
target changes.

force_nonfallthru_and_redirect is used by two functions: force_nonfallthru
and the one quoted above.  The former always passes e->dest as the second
argument, so I think it makes more sense to set BB_DIRTY in the caller
(rtl_redirect_edge_and_branch_force) rather than the callee.  This also
makes the link between BB_DIRTY and the redirect_edge_and_branch functions
more obvious.

Bootstrapped & regression tested on mips64-linux-gnu.  I checked that
it fixes the immediate point of failure for the original testcase on
an i686-pc-linux-gnu to ia64-linux-gnu cross compiler, but it then
segfaults for unrelated reasons in ia64_reorg.

I don't know which version the maintainers will prefer, but just in
case... OK to install?

Richard

PS. Is the current form of the tree threader expected to detect
    this kind of case?


gcc/
	PR rtl-optimization/26725
	* cfgrtl.c (rtl_redirect_edge_and_branch_force): Set the source
	block's BB_DIRTY flag.

gcc/testsuite/
	* gcc.c-torture/compile/pr26725.c: New test.

Index: gcc/cfgrtl.c
===================================================================
--- gcc/cfgrtl.c	(revision 112231)
+++ gcc/cfgrtl.c	(working copy)
@@ -1170,6 +1170,7 @@ rtl_redirect_edge_and_branch_force (edge
 
   /* In case the edge redirection failed, try to force it to be non-fallthru
      and redirect newly created simplejump.  */
+  e->src->flags |= BB_DIRTY;
   return force_nonfallthru_and_redirect (e, target);
 }
 
Index: gcc/testsuite/gcc.c-torture/compile/pr26725.c
===================================================================
--- gcc/testsuite/gcc.c-torture/compile/pr26725.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/compile/pr26725.c	(revision 0)
@@ -0,0 +1,15 @@
+struct { unsigned int num; } *numptr;
+void notice (int);
+void doit (unsigned int *);
+
+void
+rewrite_finalize_block (int x)
+{
+  unsigned int *tmp;
+  while (tmp = (numptr ? &numptr->num : 0), (tmp ? *tmp : 0) > 0)
+    {
+      tmp = (numptr ? &numptr->num : 0);
+      (void) (*tmp ? 0 : notice (x));
+      doit (tmp);
+    }
+}