Patchwork [4.5] Fix PR middle-end/43085 (make profiledbootstrap crashes due to dataflow bug)

login
register
mail settings
Submitter Ulrich Weigand
Date April 27, 2011, 4:27 p.m.
Message ID <201104271627.p3RGRT7B016124@d06av02.portsmouth.uk.ibm.com>
Download mbox | patch
Permalink /patch/93092/
State New
Headers show

Comments

Ulrich Weigand - April 27, 2011, 4:27 p.m.
Hello,

PR middle-end/43085 is a crash of cc1plus when building libstdc++ during
make profiledbootstrap on the 4.5 branch.  The crash is caused by a
mis-compile of the add_method routine with -fprofile-use due to a
dataflow bug during the final if-conversion pass.  For more detailed
analysis see the PR.

The dataflow bug was fixed on mainline (already for 4.6) by a series of
patches by Bernd Schmidt, who noticed the bug in a different context.

The patch below is a backport of those fixes to the 4.5 branch, which
fixes the profiled-bootstrap failure for me.  (Note that on current
mainling, the ifcvt.c dead_or_predictable routine has been significantly
rewritten beyond what was done by those patches.  These additional
changes do not appear to be necessary to fix the PR ...)

Tested on 4.5 on i386-linux with no regression; make profiledbootstrap
works again (and the resulting compiler also passes the testsuite with
no regressions).

OK for the 4.5 branch?

Bye,
Ulrich


2011-04-27  Ulrich Weigand  <ulrich.weigand@linaro.org>

	PR middle-end/43085
	Backport from mainline:

	2010-04-29  Bernd Schmidt  <bernds@codesourcery.com>

	From Dominique d'Humieres <dominiq@lps.ens.fr>
	PR bootstrap/43858
	* ifcvt.c (dead_or_predicable): Use df_simulate_find_defs to compute
	test_set.

	2010-04-26  Bernd Schmidt  <bernds@codesourcery.com>

	* df-problems.c (df_simulate_initialize_forwards): Set, don't clear,
	bits for artificial defs at the top of the block.
	* fwprop.c (single_def_use_enter_block): Don't call it.

	2010-04-22  Bernd Schmidt  <bernds@codesourcery.com>

	* ifcvt.c (dead_or_predicable): Use df_simulate_find_defs and
	df_simulate_find_noclobber_defs as appropriate.  Keep track of an
	extra set merge_set_noclobber, and use it to relax the final test
	slightly.
	* df.h (df_simulate_find_noclobber_defs): Declare.
	* df-problems.c (df_simulate_find_defs): Don't ignore partial or
	conditional defs.
	(df_simulate_find_noclobber_defs): New function.
Richard Guenther - April 27, 2011, 7:01 p.m.
On Wed, Apr 27, 2011 at 6:27 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Hello,
>
> PR middle-end/43085 is a crash of cc1plus when building libstdc++ during
> make profiledbootstrap on the 4.5 branch.  The crash is caused by a
> mis-compile of the add_method routine with -fprofile-use due to a
> dataflow bug during the final if-conversion pass.  For more detailed
> analysis see the PR.
>
> The dataflow bug was fixed on mainline (already for 4.6) by a series of
> patches by Bernd Schmidt, who noticed the bug in a different context.
>
> The patch below is a backport of those fixes to the 4.5 branch, which
> fixes the profiled-bootstrap failure for me.  (Note that on current
> mainling, the ifcvt.c dead_or_predictable routine has been significantly
> rewritten beyond what was done by those patches.  These additional
> changes do not appear to be necessary to fix the PR ...)
>
> Tested on 4.5 on i386-linux with no regression; make profiledbootstrap
> works again (and the resulting compiler also passes the testsuite with
> no regressions).
>
> OK for the 4.5 branch?

Looks ok to me from a RM perspective, once the branch is unfrozen again.

Richard.

> Bye,
> Ulrich
>
>
> 2011-04-27  Ulrich Weigand  <ulrich.weigand@linaro.org>
>
>        PR middle-end/43085
>        Backport from mainline:
>
>        2010-04-29  Bernd Schmidt  <bernds@codesourcery.com>
>
>        From Dominique d'Humieres <dominiq@lps.ens.fr>
>        PR bootstrap/43858
>        * ifcvt.c (dead_or_predicable): Use df_simulate_find_defs to compute
>        test_set.
>
>        2010-04-26  Bernd Schmidt  <bernds@codesourcery.com>
>
>        * df-problems.c (df_simulate_initialize_forwards): Set, don't clear,
>        bits for artificial defs at the top of the block.
>        * fwprop.c (single_def_use_enter_block): Don't call it.
>
>        2010-04-22  Bernd Schmidt  <bernds@codesourcery.com>
>
>        * ifcvt.c (dead_or_predicable): Use df_simulate_find_defs and
>        df_simulate_find_noclobber_defs as appropriate.  Keep track of an
>        extra set merge_set_noclobber, and use it to relax the final test
>        slightly.
>        * df.h (df_simulate_find_noclobber_defs): Declare.
>        * df-problems.c (df_simulate_find_defs): Don't ignore partial or
>        conditional defs.
>        (df_simulate_find_noclobber_defs): New function.
>
>
> Index: gcc/fwprop.c
> ===================================================================
> --- gcc/fwprop.c        (revision 172641)
> +++ gcc/fwprop.c        (working copy)
> @@ -228,8 +228,11 @@
>
>   process_uses (df_get_artificial_uses (bb_index), DF_REF_AT_TOP);
>   process_defs (df_get_artificial_defs (bb_index), DF_REF_AT_TOP);
> -  df_simulate_initialize_forwards (bb, local_lr);
>
> +  /* We don't call df_simulate_initialize_forwards, as it may overestimate
> +     the live registers if there are unused artificial defs.  We prefer
> +     liveness to be underestimated.  */
> +
>   FOR_BB_INSNS (bb, insn)
>     if (INSN_P (insn))
>       {
> Index: gcc/ifcvt.c
> ===================================================================
> --- gcc/ifcvt.c (revision 172641)
> +++ gcc/ifcvt.c (working copy)
> @@ -3818,7 +3818,7 @@
>                    basic_block other_bb, basic_block new_dest, int reversep)
>  {
>   rtx head, end, jump, earliest = NULL_RTX, old_dest, new_label = NULL_RTX;
> -  bitmap merge_set = NULL;
> +  bitmap merge_set = NULL, merge_set_noclobber = NULL;
>   /* Number of pending changes.  */
>   int n_validated_changes = 0;
>
> @@ -3951,11 +3951,14 @@
>
>       /* Collect:
>           MERGE_SET = set of registers set in MERGE_BB
> +          MERGE_SET_NOCLOBBER = like MERGE_SET, but only includes registers
> +            that are really set, not just clobbered.
>           TEST_LIVE = set of registers live at EARLIEST
> -          TEST_SET  = set of registers set between EARLIEST and the
> -                      end of the block.  */
> +          TEST_SET = set of registers set between EARLIEST and the
> +            end of the block.  */
>
>       merge_set = BITMAP_ALLOC (&reg_obstack);
> +      merge_set_noclobber = BITMAP_ALLOC (&reg_obstack);
>
>       /* If we allocated new pseudos (e.g. in the conditional move
>         expander called from noce_emit_cmove), we must resize the
> @@ -3967,13 +3970,8 @@
>        {
>          if (NONDEBUG_INSN_P (insn))
>            {
> -             unsigned int uid = INSN_UID (insn);
> -             df_ref *def_rec;
> -             for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
> -               {
> -                 df_ref def = *def_rec;
> -                 bitmap_set_bit (merge_set, DF_REF_REGNO (def));
> -               }
> +             df_simulate_find_defs (insn, merge_set);
> +             df_simulate_find_noclobber_defs (insn, merge_set_noclobber);
>            }
>        }
>
> @@ -3984,7 +3982,7 @@
>          unsigned i;
>          bitmap_iterator bi;
>
> -          EXECUTE_IF_SET_IN_BITMAP (merge_set, 0, i, bi)
> +          EXECUTE_IF_SET_IN_BITMAP (merge_set_noclobber, 0, i, bi)
>            {
>              if (i < FIRST_PSEUDO_REGISTER
>                  && ! fixed_regs[i]
> @@ -4015,12 +4013,14 @@
>        }
>
>       /* We can perform the transformation if
> -          MERGE_SET & (TEST_SET | TEST_LIVE)
> +          MERGE_SET_NOCLOBBER & TEST_SET
>         and
> +          MERGE_SET & TEST_LIVE
> +        and
>           TEST_SET & DF_LIVE_IN (merge_bb)
>         are empty.  */
>
> -      if (bitmap_intersect_p (merge_set, test_set)
> +      if (bitmap_intersect_p (merge_set_noclobber, test_set)
>          || bitmap_intersect_p (merge_set, test_live)
>          || bitmap_intersect_p (test_set, df_get_live_in (merge_bb)))
>        intersect = true;
> @@ -4104,10 +4104,11 @@
>          unsigned i;
>          bitmap_iterator bi;
>
> -         EXECUTE_IF_SET_IN_BITMAP (merge_set, 0, i, bi)
> +         EXECUTE_IF_SET_IN_BITMAP (merge_set_noclobber, 0, i, bi)
>            remove_reg_equal_equiv_notes_for_regno (i);
>
>          BITMAP_FREE (merge_set);
> +         BITMAP_FREE (merge_set_noclobber);
>        }
>
>       reorder_insns (head, end, PREV_INSN (earliest));
> @@ -4128,7 +4129,10 @@
>   cancel_changes (0);
>  fail:
>   if (merge_set)
> -    BITMAP_FREE (merge_set);
> +    {
> +      BITMAP_FREE (merge_set);
> +      BITMAP_FREE (merge_set_noclobber);
> +    }
>   return FALSE;
>  }
>
> Index: gcc/df.h
> ===================================================================
> --- gcc/df.h    (revision 172641)
> +++ gcc/df.h    (working copy)
> @@ -978,6 +978,7 @@
>  extern void df_md_add_problem (void);
>  extern void df_md_simulate_artificial_defs_at_top (basic_block, bitmap);
>  extern void df_md_simulate_one_insn (basic_block, rtx, bitmap);
> +extern void df_simulate_find_noclobber_defs (rtx, bitmap);
>  extern void df_simulate_find_defs (rtx, bitmap);
>  extern void df_simulate_defs (rtx, bitmap);
>  extern void df_simulate_uses (rtx, bitmap);
> Index: gcc/df-problems.c
> ===================================================================
> --- gcc/df-problems.c   (revision 172641)
> +++ gcc/df-problems.c   (working copy)
> @@ -3748,9 +3748,22 @@
>   for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
>     {
>       df_ref def = *def_rec;
> -      /* If the def is to only part of the reg, it does
> -        not kill the other defs that reach here.  */
> -      if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
> +      bitmap_set_bit (defs, DF_REF_REGNO (def));
> +    }
> +}
> +
> +/* Find the set of real DEFs, which are not clobbers, for INSN.  */
> +
> +void
> +df_simulate_find_noclobber_defs (rtx insn, bitmap defs)
> +{
> +  df_ref *def_rec;
> +  unsigned int uid = INSN_UID (insn);
> +
> +  for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
> +    {
> +      df_ref def = *def_rec;
> +      if (!(DF_REF_FLAGS (def) & (DF_REF_MUST_CLOBBER | DF_REF_MAY_CLOBBER)))
>        bitmap_set_bit (defs, DF_REF_REGNO (def));
>     }
>  }
> @@ -3903,13 +3916,9 @@
>    the block, starting with the first one.
>    ----------------------------------------------------------------------------*/
>
> -/* Apply the artificial uses and defs at the top of BB in a forwards
> -   direction.  ??? This is wrong; defs mark the point where a pseudo
> -   becomes live when scanning forwards (unless a def is unused).  Since
> -   there are no REG_UNUSED notes for artificial defs, passes that
> -   require artificial defs probably should not call this function
> -   unless (as is the case for fwprop) they are correct when liveness
> -   bitmaps are *under*estimated.  */
> +/* Initialize the LIVE bitmap, which should be copied from DF_LIVE_IN or
> +   DF_LR_IN for basic block BB, for forward scanning by marking artificial
> +   defs live.  */
>
>  void
>  df_simulate_initialize_forwards (basic_block bb, bitmap live)
> @@ -3921,7 +3930,7 @@
>     {
>       df_ref def = *def_rec;
>       if (DF_REF_FLAGS (def) & DF_REF_AT_TOP)
> -       bitmap_clear_bit (live, DF_REF_REGNO (def));
> +       bitmap_set_bit (live, DF_REF_REGNO (def));
>     }
>  }
>
> @@ -3942,7 +3951,7 @@
>      while here the scan is performed forwards!  So, first assume that the
>      def is live, and if this is not true REG_UNUSED notes will rectify the
>      situation.  */
> -  df_simulate_find_defs (insn, live);
> +  df_simulate_find_noclobber_defs (insn, live);
>
>   /* Clear all of the registers that go dead.  */
>   for (link = REG_NOTES (insn); link; link = XEXP (link, 1))
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>
Eric Botcazou - April 28, 2011, 4:04 p.m.
> The patch below is a backport of those fixes to the 4.5 branch, which
> fixes the profiled-bootstrap failure for me.  (Note that on current
> mainling, the ifcvt.c dead_or_predictable routine has been significantly
> rewritten beyond what was done by those patches.  These additional
> changes do not appear to be necessary to fix the PR ...)

Do you really need to backport the MERGE_SET_NOCLOBBER stuff?  This will cause 
the branch to optimize more aggressively than before.

Patch

Index: gcc/fwprop.c
===================================================================
--- gcc/fwprop.c	(revision 172641)
+++ gcc/fwprop.c	(working copy)
@@ -228,8 +228,11 @@ 
 
   process_uses (df_get_artificial_uses (bb_index), DF_REF_AT_TOP);
   process_defs (df_get_artificial_defs (bb_index), DF_REF_AT_TOP);
-  df_simulate_initialize_forwards (bb, local_lr);
 
+  /* We don't call df_simulate_initialize_forwards, as it may overestimate
+     the live registers if there are unused artificial defs.  We prefer
+     liveness to be underestimated.  */
+
   FOR_BB_INSNS (bb, insn)
     if (INSN_P (insn))
       {
Index: gcc/ifcvt.c
===================================================================
--- gcc/ifcvt.c	(revision 172641)
+++ gcc/ifcvt.c	(working copy)
@@ -3818,7 +3818,7 @@ 
 		    basic_block other_bb, basic_block new_dest, int reversep)
 {
   rtx head, end, jump, earliest = NULL_RTX, old_dest, new_label = NULL_RTX;
-  bitmap merge_set = NULL;
+  bitmap merge_set = NULL, merge_set_noclobber = NULL;
   /* Number of pending changes.  */
   int n_validated_changes = 0;
 
@@ -3951,11 +3951,14 @@ 
 
       /* Collect:
 	   MERGE_SET = set of registers set in MERGE_BB
+	   MERGE_SET_NOCLOBBER = like MERGE_SET, but only includes registers
+	     that are really set, not just clobbered.
 	   TEST_LIVE = set of registers live at EARLIEST
-	   TEST_SET  = set of registers set between EARLIEST and the
-		       end of the block.  */
+	   TEST_SET = set of registers set between EARLIEST and the
+	     end of the block.  */
 
       merge_set = BITMAP_ALLOC (&reg_obstack);
+      merge_set_noclobber = BITMAP_ALLOC (&reg_obstack);
 
       /* If we allocated new pseudos (e.g. in the conditional move
 	 expander called from noce_emit_cmove), we must resize the
@@ -3967,13 +3970,8 @@ 
 	{
 	  if (NONDEBUG_INSN_P (insn))
 	    {
-	      unsigned int uid = INSN_UID (insn);
-	      df_ref *def_rec;
-	      for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
-		{
-		  df_ref def = *def_rec;
-		  bitmap_set_bit (merge_set, DF_REF_REGNO (def));
-		}
+	      df_simulate_find_defs (insn, merge_set);
+	      df_simulate_find_noclobber_defs (insn, merge_set_noclobber);
 	    }
 	}
 
@@ -3984,7 +3982,7 @@ 
 	  unsigned i;
 	  bitmap_iterator bi;
 
-          EXECUTE_IF_SET_IN_BITMAP (merge_set, 0, i, bi)
+          EXECUTE_IF_SET_IN_BITMAP (merge_set_noclobber, 0, i, bi)
 	    {
 	      if (i < FIRST_PSEUDO_REGISTER
 		  && ! fixed_regs[i]
@@ -4015,12 +4013,14 @@ 
 	}
 
       /* We can perform the transformation if
-	   MERGE_SET & (TEST_SET | TEST_LIVE)
+	   MERGE_SET_NOCLOBBER & TEST_SET
 	 and
+	   MERGE_SET & TEST_LIVE
+	 and
 	   TEST_SET & DF_LIVE_IN (merge_bb)
 	 are empty.  */
 
-      if (bitmap_intersect_p (merge_set, test_set)
+      if (bitmap_intersect_p (merge_set_noclobber, test_set)
 	  || bitmap_intersect_p (merge_set, test_live)
 	  || bitmap_intersect_p (test_set, df_get_live_in (merge_bb)))
 	intersect = true;
@@ -4104,10 +4104,11 @@ 
 	  unsigned i;
 	  bitmap_iterator bi;
 
-	  EXECUTE_IF_SET_IN_BITMAP (merge_set, 0, i, bi)
+	  EXECUTE_IF_SET_IN_BITMAP (merge_set_noclobber, 0, i, bi)
 	    remove_reg_equal_equiv_notes_for_regno (i);
 
 	  BITMAP_FREE (merge_set);
+	  BITMAP_FREE (merge_set_noclobber);
 	}
 
       reorder_insns (head, end, PREV_INSN (earliest));
@@ -4128,7 +4129,10 @@ 
   cancel_changes (0);
  fail:
   if (merge_set)
-    BITMAP_FREE (merge_set);
+    {
+      BITMAP_FREE (merge_set);
+      BITMAP_FREE (merge_set_noclobber);
+    }
   return FALSE;
 }
 
Index: gcc/df.h
===================================================================
--- gcc/df.h	(revision 172641)
+++ gcc/df.h	(working copy)
@@ -978,6 +978,7 @@ 
 extern void df_md_add_problem (void);
 extern void df_md_simulate_artificial_defs_at_top (basic_block, bitmap);
 extern void df_md_simulate_one_insn (basic_block, rtx, bitmap);
+extern void df_simulate_find_noclobber_defs (rtx, bitmap);
 extern void df_simulate_find_defs (rtx, bitmap);
 extern void df_simulate_defs (rtx, bitmap);
 extern void df_simulate_uses (rtx, bitmap);
Index: gcc/df-problems.c
===================================================================
--- gcc/df-problems.c	(revision 172641)
+++ gcc/df-problems.c	(working copy)
@@ -3748,9 +3748,22 @@ 
   for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
     {
       df_ref def = *def_rec;
-      /* If the def is to only part of the reg, it does
-	 not kill the other defs that reach here.  */
-      if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
+      bitmap_set_bit (defs, DF_REF_REGNO (def));
+    }
+}
+
+/* Find the set of real DEFs, which are not clobbers, for INSN.  */
+
+void
+df_simulate_find_noclobber_defs (rtx insn, bitmap defs)
+{
+  df_ref *def_rec;
+  unsigned int uid = INSN_UID (insn);
+
+  for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
+    {
+      df_ref def = *def_rec;
+      if (!(DF_REF_FLAGS (def) & (DF_REF_MUST_CLOBBER | DF_REF_MAY_CLOBBER)))
 	bitmap_set_bit (defs, DF_REF_REGNO (def));
     }
 }
@@ -3903,13 +3916,9 @@ 
    the block, starting with the first one.
    ----------------------------------------------------------------------------*/
 
-/* Apply the artificial uses and defs at the top of BB in a forwards
-   direction.  ??? This is wrong; defs mark the point where a pseudo
-   becomes live when scanning forwards (unless a def is unused).  Since
-   there are no REG_UNUSED notes for artificial defs, passes that
-   require artificial defs probably should not call this function
-   unless (as is the case for fwprop) they are correct when liveness
-   bitmaps are *under*estimated.  */
+/* Initialize the LIVE bitmap, which should be copied from DF_LIVE_IN or
+   DF_LR_IN for basic block BB, for forward scanning by marking artificial
+   defs live.  */
 
 void
 df_simulate_initialize_forwards (basic_block bb, bitmap live)
@@ -3921,7 +3930,7 @@ 
     {
       df_ref def = *def_rec;
       if (DF_REF_FLAGS (def) & DF_REF_AT_TOP)
-	bitmap_clear_bit (live, DF_REF_REGNO (def));
+	bitmap_set_bit (live, DF_REF_REGNO (def));
     }
 }
 
@@ -3942,7 +3951,7 @@ 
      while here the scan is performed forwards!  So, first assume that the
      def is live, and if this is not true REG_UNUSED notes will rectify the
      situation.  */
-  df_simulate_find_defs (insn, live);
+  df_simulate_find_noclobber_defs (insn, live);
 
   /* Clear all of the registers that go dead.  */
   for (link = REG_NOTES (insn); link; link = XEXP (link, 1))