Re: [patch, 4.5] Fix PR middle-end/43085 (make profiledbootstrap crashes due to dataflow bug)

[Richard Guenther <richard dot guenther at gmail dot com>]

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
>