On Wed, 13 Dec 2017, Jakub Jelinek wrote:
> On Wed, Dec 13, 2017 at 11:45:51AM +0100, Jakub Jelinek wrote:
> > On Wed, Dec 13, 2017 at 10:28:22AM +0100, Jakub Jelinek wrote:
> > > 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,
> >
> > Here it is everything in patch form, in case some volunteers are willing to
> > test it on their targets, because we need faster turn-arounds for this.
> >
> > 2017-12-13 Alexandre Oliva <[email protected]>
> > Jakub Jelinek <[email protected]>
> >
> > PR bootstrap/83396
> > PR debug/83391
> > * tree-cfgcleanup.c (remove_forwarder_block): Keep after
> > labels debug stmts that can only appear after labels.
> >
> > * gcc.dg/torture/pr83396.c: New test.
> > * g++.dg/torture/pr83391.C: New test.
>
> Bootstrapped/regtested on x86_64-linux, i686-linux and powerpc64le-linux,
> powerpc64-linux regtest still pending, ok for trunk?
Ok.
Richard.
> > --- gcc/tree-cfgcleanup.c.jj 2017-12-12 09:48:26.813393301 +0100
> > +++ gcc/tree-cfgcleanup.c 2017-12-13 11:39:03.373065381 +0100
> > @@ -536,9 +536,14 @@ 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 in between
> > + labels, but not those that can only appear after labels. */
> > 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);
> > + while (gsi_stmt (gsi) != gsi_stmt (gsie))
> > {
> > tree decl;
> > label = gsi_stmt (gsi);
> > @@ -557,6 +562,21 @@ 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));
> > + gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
> > + 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. */
> > --- gcc/testsuite/g++.dg/torture/pr83391.C.jj
> > +++ gcc/testsuite/g++.dg/torture/pr83391.C
> > @@ -0,0 +1,36 @@
> > +// PR debug/83391
> > +// { dg-do compile }
> > +// { dg-options "-g" }
> > +// { dg-additional-options "-mbranch-cost=1" { target { i?86-*-*
> > x86_64-*-* mips*-*-* s390*-*-* avr*-*-* } } }
> > +
> > +unsigned char a;
> > +enum E { F, G, H } b;
> > +int c, d;
> > +
> > +void
> > +foo ()
> > +{
> > + int e;
> > + bool f;
> > + E g = b;
> > + while (1)
> > + {
> > + unsigned char h = a ? d : 0;
> > + switch (g)
> > + {
> > + case 0:
> > + f = h <= 'Z' || h >= 'a' && h <= 'z';
> > + break;
> > + case 1:
> > + {
> > + unsigned char i = h;
> > + e = 0;
> > + }
> > + if (e || h)
> > + g = H;
> > + /* FALLTHRU */
> > + default:
> > + c = 0;
> > + }
> > + }
> > +}
> > --- gcc/testsuite/gcc.dg/torture/pr83396.c.jj
> > +++ gcc/testsuite/gcc.dg/torture/pr83396.c
> > @@ -0,0 +1,38 @@
> > +/* PR bootstrap/83396 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-g" } */
> > +
> > +int fn1 (void);
> > +void fn2 (void *, const char *);
> > +void fn3 (void);
> > +
> > +void
> > +fn4 (long long x)
> > +{
> > + fn3 ();
> > +}
> > +
> > +void
> > +fn5 (long long x)
> > +{
> > + if (x)
> > + fn3();
> > +}
> > +
> > +void
> > +fn6 (long long x)
> > +{
> > + switch (fn1 ())
> > + {
> > + case 0:
> > + fn5 (x);
> > + case 2:
> > + fn2 (0, "");
> > + break;
> > + case 1:
> > + case 3:
> > + fn4(x);
> > + case 5:
> > + fn2 (0, "");
> > + }
> > +}
> >
> >
> > Jakub
>
> Jakub
>
>
--
Richard Biener <[email protected]>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
