On Wed, 13 Sep 2017, Jakub Jelinek wrote:
> Hi!
>
> The following testcase fails on powerpc64le with -fcompare-debug.
> The problem is that find_many_sub_basic_blocks is called during
> expansion on a basic block where originally two __atomic_fetch_add
> calls have some debug stmts in between. The rs6000 backend expands
> those atomic calls into a loop that starts with a CODE_LABEL and
> ends with a JUMP_INSN, so find_many_sub_basic_blocks must
> end a basic block after the JUMP_INSN from the first call and
> start a new basic block at the start of the second call.
> If there are no DEBUG_INSNs in between, it creates just those 2
> basic blocks, which is correct, but it doesn't ignore DEBUG_INSNs when
> deciding what to do and thus when there are DEBUG_ISNSs in between
> the flow_transfer_insn (JUMP_INSN) and CODE_LABEL, it creates yet
> another basic block that has just the DEBUG_INSNs in it. That means
> different cfg between -g and -g0 and more differences later on.
>
> As I wrote in the PR, if we have 2 loops and some code in between
> at GIMPLE, and in that code in between DCE all the non-debug stmts and
> have only debug stmts in there, we consider it a forwarder block and delete
> it with all those debug stmts (it is one of the unfortunate cases where we
> drop them on the floor, but there isn't really much we can do with those
> without adding new extensions like conditional debug stmts or debug phis).
>
> The following patch attempts to do the same thing on RTL during
> find_many_sub_basic_blocks, i.e. ignore debug stmts when deciding what to
> do, trying to preserve debug insns somewhere if that is easily possible
> (if there is say a conditional jump with a fallthrough into debug insns
> followed by some real insn (not a CODE_LABEL nor BARRIER), the debug
> insns can go at the start of the fallthrough bb that would be created
> anyway). If we have an insn that must end a bb and another one that must
> start a bb (CODE_LABEL) and only debug insns in between, we remove them.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux (in these two cases
> I've also gathered statistics and no debug insns during the
> bootstraps/regtests were actually removed) and powerpc64le-linux (forgot to
> apply the statistics patch, so no details, but it surely triggers at least
> on the testcase). Ok for trunk?
Ok.
Thanks,
Richard.
> 2017-09-13 Jakub Jelinek <ja...@redhat.com>
>
> PR target/81325
> * cfgbuild.c (find_bb_boundaries): Ignore debug insns in decisions
> if and where to split a bb, except for splitting before debug insn
> sequences followed by non-label real insn. Delete debug insns
> in between basic blocks.
>
> * g++.dg/cpp0x/pr81325.C: New test.
>
> --- gcc/cfgbuild.c.jj 2017-09-12 21:57:53.084514168 +0200
> +++ gcc/cfgbuild.c 2017-09-13 09:15:57.701521179 +0200
> @@ -442,9 +442,10 @@ find_bb_boundaries (basic_block bb)
> rtx_insn *end = BB_END (bb), *x;
> rtx_jump_table_data *table;
> rtx_insn *flow_transfer_insn = NULL;
> + rtx_insn *debug_insn = NULL;
> edge fallthru = NULL;
>
> - if (insn == BB_END (bb))
> + if (insn == end)
> return;
>
> if (LABEL_P (insn))
> @@ -455,22 +456,43 @@ find_bb_boundaries (basic_block bb)
> {
> enum rtx_code code = GET_CODE (insn);
>
> + if (code == DEBUG_INSN)
> + {
> + if (flow_transfer_insn && !debug_insn)
> + debug_insn = insn;
> + }
> /* In case we've previously seen an insn that effects a control
> flow transfer, split the block. */
> - if ((flow_transfer_insn || code == CODE_LABEL)
> - && inside_basic_block_p (insn))
> + else if ((flow_transfer_insn || code == CODE_LABEL)
> + && inside_basic_block_p (insn))
> {
> - fallthru = split_block (bb, PREV_INSN (insn));
> + rtx_insn *prev = PREV_INSN (insn);
> +
> + /* If the first non-debug inside_basic_block_p insn after a control
> + flow transfer is not a label, split the block before the debug
> + insn instead of before the non-debug insn, so that the debug
> + insns are not lost. */
> + if (debug_insn && code != CODE_LABEL && code != BARRIER)
> + prev = PREV_INSN (debug_insn);
> + fallthru = split_block (bb, prev);
> if (flow_transfer_insn)
> {
> BB_END (bb) = flow_transfer_insn;
>
> + rtx_insn *next;
> /* Clean up the bb field for the insns between the blocks. */
> for (x = NEXT_INSN (flow_transfer_insn);
> x != BB_HEAD (fallthru->dest);
> - x = NEXT_INSN (x))
> - if (!BARRIER_P (x))
> - set_block_for_insn (x, NULL);
> + x = next)
> + {
> + next = NEXT_INSN (x);
> + /* Debug insns should not be in between basic blocks,
> + drop them on the floor. */
> + if (DEBUG_INSN_P (x))
> + delete_insn (x);
> + else if (!BARRIER_P (x))
> + set_block_for_insn (x, NULL);
> + }
> }
>
> bb = fallthru->dest;
> @@ -480,6 +502,7 @@ find_bb_boundaries (basic_block bb)
> bb->frequency = 0;
> bb->count = profile_count::uninitialized ();
> flow_transfer_insn = NULL;
> + debug_insn = NULL;
> if (code == CODE_LABEL && LABEL_ALT_ENTRY_P (insn))
> make_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun), bb, 0);
> }
> @@ -502,17 +525,23 @@ find_bb_boundaries (basic_block bb)
> /* In case expander replaced normal insn by sequence terminating by
> return and barrier, or possibly other sequence not behaving like
> ordinary jump, we need to take care and move basic block boundary. */
> - if (flow_transfer_insn)
> + if (flow_transfer_insn && flow_transfer_insn != end)
> {
> BB_END (bb) = flow_transfer_insn;
>
> /* Clean up the bb field for the insns that do not belong to BB. */
> - x = flow_transfer_insn;
> - while (x != end)
> + rtx_insn *next;
> + for (x = NEXT_INSN (flow_transfer_insn); ; x = next)
> {
> - x = NEXT_INSN (x);
> - if (!BARRIER_P (x))
> + next = NEXT_INSN (x);
> + /* Debug insns should not be in between basic blocks,
> + drop them on the floor. */
> + if (DEBUG_INSN_P (x))
> + delete_insn (x);
> + else if (!BARRIER_P (x))
> set_block_for_insn (x, NULL);
> + if (x == end)
> + break;
> }
> }
>
> --- gcc/testsuite/g++.dg/cpp0x/pr81325.C.jj 2017-09-12 22:17:54.662002067
> +0200
> +++ gcc/testsuite/g++.dg/cpp0x/pr81325.C 2017-09-13 09:21:44.536059474
> +0200
> @@ -0,0 +1,84 @@
> +// PR target/81325
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -fcompare-debug" }
> +
> +struct A { A(const char *, const int & = 0); };
> +template <typename> struct B;
> +template <typename> struct C {
> + int _M_i;
> + void m_fn1() { __atomic_fetch_add(&_M_i, 0, __ATOMIC_RELAXED); }
> +};
> +struct D {
> + int *Data;
> + long Length = 0;
> + D(int) : Data() {}
> +};
> +template <> struct B<int> : C<int> {};
> +struct F {
> + B<int> RefCount;
> + void m_fn2() { RefCount.m_fn1(); }
> +};
> +struct G {
> + F *Obj;
> + G(const G &p1) : Obj(p1.Obj) {
> + if (Obj) {
> + F *a = 0;
> + a->m_fn2();
> + }
> + }
> +};
> +struct H {
> + int CPlusPlus : 1;
> +};
> +struct I {
> + enum {} KindId;
> +};
> +template <typename ResultT, typename ArgT> struct J {
> + void operator()();
> + ResultT operator()(ArgT) {}
> +};
> +struct K {
> + int AllowBind;
> + I SupportedKind;
> + I RestrictKind;
> + G Implementation;
> +};
> +struct L {
> + L(int) : Implementation(Implementation) {}
> + K Implementation;
> +};
> +struct M {
> + int Param1;
> +};
> +struct N {
> + N(int, L &p2) : Param2(p2) {}
> + L Param2;
> +};
> +struct O {
> + L m_fn3();
> +};
> +L ignoringImpCasts(L);
> +J<O, L> b;
> +L hasName(const A &);
> +M hasOverloadedOperatorName(D);
> +J<O, int> c;
> +struct P {
> + void m_fn4(L, int);
> +};
> +struct Q {
> + void m_fn5(P *);
> +};
> +H d;
> +void Q::m_fn5(P *p1) {
> + if (!d.CPlusPlus) {
> + c();
> + L e = 0, f = ignoringImpCasts(e);
> + b(ignoringImpCasts(f)).m_fn3();
> + }
> + hasOverloadedOperatorName(0);
> + hasName("");
> + L g = 0;
> + N(0, g);
> + L h(0);
> + p1->m_fn4(h, 0);
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
