On Wed, Dec 13, 2017 at 05:21:01AM -0200, Alexandre Oliva wrote:
> index 000000000000..098a1101a3f4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr83391.C
> @@ -0,0 +1,34 @@
> +/* PR debug/83391 */
> +/* { dg-do compile } */
If you put this into dg-torture.exp, please add:
/* { dg-options "-g" } */
and readd the needed:
/* { dg-additional-options "-mbranch-cost=1" { target { i?86-*-* x86_64-*-*
mips*-*-* s390*-*-* avr*-*-* } } } */
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr83396.c
> @@ -0,0 +1,37 @@
> +/* PR bootstrap/83396 */
> +/* { dg-do compile } */
Please add -g.
> --- a/gcc/tree-cfgcleanup.c
> +++ b/gcc/tree-cfgcleanup.c
> @@ -536,14 +536,23 @@ remove_forwarder_block (basic_block bb)
> defined labels and labels with an EH landing pad number to the
> new block, so that the redirection of the abnormal edges works,
> jump targets end up in a sane place and debug information for
> - labels is retained. */
> + labels is retained.
> +
> + While at that, move any debug stmts that appear before or among
> + labels, but not those that can only appear after labels, unless
> + the destination block didn't have labels of its own. */
> gsi_to = gsi_start_bb (dest);
> - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> + gsi = gsi_start_bb (bb);
> + gimple_stmt_iterator gsie = gsi_after_labels (bb);
> + gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
> + bool can_move_early_debug_stmts = can_move_debug_stmts
> + && (gsi_stmt (gsi) != gsi_stmt (gsie) || gsi_stmt (gsi_to) != gsi_stmt
> (gsie_to));
Formatting, this should be
bool can_move_early_debug_stmts
= ...
and the line is too long, so needs to be wrapped.
Furthermore, I must say I don't understand why
can_move_early_debug_stmts should care whether there are any labels in
dest bb or not. That sounds very risky for introducing non-# DEBUG BEGIN_STMT
debug insns before labels if it could happen. Though, if
gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
do anything and nothing cares about can_move_early_debug_stmts afterwards.
So, in short, can_move_early_debug_stmts is used only if
gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);
So, can we get rid of can_move_early_debug_stmts altogether and just use
can_move_debug_stmts in there instead?
Another thing I find risky is that you compute gsie_to so early and don't
update it. If you don't need it above for can_move_early_debug_stmts, can
you just do it back where it used to be done,
> + while (gsi_stmt (gsi) != gsi_stmt (gsie))
> {
> tree decl;
> label = gsi_stmt (gsi);
> if (is_gimple_debug (label)
> - ? can_move_debug_stmts
> + ? can_move_early_debug_stmts
> : ((decl = gimple_label_label (as_a <glabel *> (label))),
> EH_LANDING_PAD_NR (decl) != 0
> || DECL_NONLOCAL (decl)
> @@ -557,6 +566,20 @@ remove_forwarder_block (basic_block bb)
> gsi_next (&gsi);
> }
>
> + /* Move debug statements if the destination has a single predecessor. */
> + if (can_move_debug_stmts && !gsi_end_p (gsi))
> + {
> + gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
i.e. here?
> + do
> + {
> + gimple *debug = gsi_stmt (gsi);
> + gcc_assert (is_gimple_debug (debug));
> + gsi_remove (&gsi, false);
> + gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
> + }
> + while (!gsi_end_p (gsi));
> + }
> +
> bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
>
> /* Update the dominators. */
Jakub
