cfgbuild: Fix DEBUG_INSN handling in find_bb_boundaries [PR106719]

Message ID Y5G4RyqhX5c0p38G@tucnak
State Unresolved
Headers
Series cfgbuild: Fix DEBUG_INSN handling in find_bb_boundaries [PR106719] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Dec. 8, 2022, 10:11 a.m. UTC
  Hi!

The following testcase FAILs on aarch64-linux.  We have some atomic
instruction followed by 2 DEBUG_INSNs (if -g only of course) followed
by NOTE_INSN_EPILOGUE_BEG followed by some USE insn.
Now, split3 pass replaces the atomic instruction with a code sequence
which ends with a conditional jump and the split3 pass calls
find_many_sub_basic_blocks.
For -g0, find_bb_boundaries sees the flow_transfer_insn (the new conditional
jump), then NOTE_INSN_EPILOGUE_BEG which can live in between basic blocks
and then the USE insn, so splits block after the NOTE_INSN_EPILOGUE_BEG
and puts the NOTE in between the blocks.
For -g, if sees a DEBUG_INSN after the flow_transfer_insn, so sets
debug_insn to it, then walks over another DEBUG_INSN, NOTE_INSN_EPILOGUE_BEG
until it finally sees the USE insn, and triggers the:
          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);
code I've added for PR81325.  If there are only DEBUG_INSNs, that is
the right thing to do, but if in between debug_insn and insn there are
notes which can stay in between basic blocks or simnilarly JUMP_TABLE_DATA
or their associated CODE_LABELs, it causes -fcompare-debug differences.

The following patch fixes it by clearing debug_insn if JUMP_TABLE_DATA
or associated CODE_LABEL is seen (I'm afraid there is no good answer
what to do with DEBUG_INSNs before those; the code then removes them:
              /* 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)
                {
                  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);
                }
but if there are NOTEs, the patch just reorders the NOTEs and DEBUG_INSNs,
such that the NOTEs come first (so that they stay in between basic blocks
like with -g0) and DEBUG_INSNs after those (so that bb is split before
them, so they will be in the basic block after NOTE_INSN_BASIC_BLOCK).

Bootstrapped/regtested on x86_64-linux and i686-linux plus tested on
the testcase in a cross to aarch64-linux, ok for trunk?

2022-12-08  Jakub Jelinek  <jakub@redhat.com>

	PR debug/106719
	* cfgbuild.cc (find_bb_boundaries): If there are NOTEs in between
	debug_insn (seen after flow_transfer_insn) and insn, move NOTEs
	before all the DEBUG_INSNs and split after NOTEs.  If there are
	other insns like jump table data, clear debug_insn.

	* gcc.dg/pr106719.c: New test.


	Jakub
  

Comments

Richard Biener Dec. 8, 2022, 10:31 a.m. UTC | #1
On Thu, Dec 8, 2022 at 11:12 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> The following testcase FAILs on aarch64-linux.  We have some atomic
> instruction followed by 2 DEBUG_INSNs (if -g only of course) followed
> by NOTE_INSN_EPILOGUE_BEG followed by some USE insn.
> Now, split3 pass replaces the atomic instruction with a code sequence
> which ends with a conditional jump and the split3 pass calls
> find_many_sub_basic_blocks.
> For -g0, find_bb_boundaries sees the flow_transfer_insn (the new conditional
> jump), then NOTE_INSN_EPILOGUE_BEG which can live in between basic blocks
> and then the USE insn, so splits block after the NOTE_INSN_EPILOGUE_BEG
> and puts the NOTE in between the blocks.
> For -g, if sees a DEBUG_INSN after the flow_transfer_insn, so sets
> debug_insn to it, then walks over another DEBUG_INSN, NOTE_INSN_EPILOGUE_BEG
> until it finally sees the USE insn, and triggers the:
>           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);
> code I've added for PR81325.  If there are only DEBUG_INSNs, that is
> the right thing to do, but if in between debug_insn and insn there are
> notes which can stay in between basic blocks or simnilarly JUMP_TABLE_DATA
> or their associated CODE_LABELs, it causes -fcompare-debug differences.
>
> The following patch fixes it by clearing debug_insn if JUMP_TABLE_DATA
> or associated CODE_LABEL is seen (I'm afraid there is no good answer
> what to do with DEBUG_INSNs before those; the code then removes them:
>               /* 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)
>                 {
>                   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);
>                 }
> but if there are NOTEs, the patch just reorders the NOTEs and DEBUG_INSNs,
> such that the NOTEs come first (so that they stay in between basic blocks
> like with -g0) and DEBUG_INSNs after those (so that bb is split before
> them, so they will be in the basic block after NOTE_INSN_BASIC_BLOCK).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux plus tested on
> the testcase in a cross to aarch64-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2022-12-08  Jakub Jelinek  <jakub@redhat.com>
>
>         PR debug/106719
>         * cfgbuild.cc (find_bb_boundaries): If there are NOTEs in between
>         debug_insn (seen after flow_transfer_insn) and insn, move NOTEs
>         before all the DEBUG_INSNs and split after NOTEs.  If there are
>         other insns like jump table data, clear debug_insn.
>
>         * gcc.dg/pr106719.c: New test.
>
> --- gcc/cfgbuild.cc.jj  2022-01-18 11:58:58.944991171 +0100
> +++ gcc/cfgbuild.cc     2022-12-07 21:36:27.493363173 +0100
> @@ -445,6 +445,7 @@ find_bb_boundaries (basic_block bb)
>    rtx_insn *debug_insn = NULL;
>    edge fallthru = NULL;
>    bool skip_purge;
> +  bool seen_note_after_debug = false;
>
>    if (insn == end)
>      return;
> @@ -492,7 +493,10 @@ find_bb_boundaries (basic_block bb)
>        if (code == DEBUG_INSN)
>         {
>           if (flow_transfer_insn && !debug_insn)
> -           debug_insn = insn;
> +           {
> +             debug_insn = insn;
> +             seen_note_after_debug = false;
> +           }
>         }
>        /* In case we've previously seen an insn that effects a control
>          flow transfer, split the block.  */
> @@ -506,7 +510,40 @@ find_bb_boundaries (basic_block bb)
>              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);
> +           {
> +             prev = PREV_INSN (debug_insn);
> +             if (seen_note_after_debug)
> +               {
> +                 /* Though, if there are NOTEs intermixed with DEBUG_INSNs,
> +                    move the NOTEs before the DEBUG_INSNs and split after
> +                    the last NOTE.  */
> +                 rtx_insn *first = NULL, *last = NULL;
> +                 for (x = debug_insn; x != insn; x = NEXT_INSN (x))
> +                   {
> +                     if (NOTE_P (x))
> +                       {
> +                         if (first == NULL)
> +                           first = x;
> +                         last = x;
> +                       }
> +                     else
> +                       {
> +                         gcc_assert (DEBUG_INSN_P (x));
> +                         if (first)
> +                           {
> +                             reorder_insns_nobb (first, last, prev);
> +                             prev = last;
> +                             first = last = NULL;
> +                           }
> +                       }
> +                   }
> +                 if (first)
> +                   {
> +                     reorder_insns_nobb (first, last, prev);
> +                     prev = last;
> +                   }
> +               }
> +           }
>           fallthru = split_block (bb, prev);
>           if (flow_transfer_insn)
>             {
> @@ -547,6 +584,14 @@ find_bb_boundaries (basic_block bb)
>             flow_transfer_insn = prev_nonnote_nondebug_insn_bb (insn);
>           debug_insn = NULL;
>         }
> +      else if (debug_insn)
> +       {
> +         if (code == NOTE)
> +           seen_note_after_debug = true;
> +         else
> +           /* Jump tables.  */
> +           debug_insn = NULL;
> +       }
>
>        if (control_flow_insn_p (insn))
>         flow_transfer_insn = insn;
> --- gcc/testsuite/gcc.dg/pr106719.c.jj  2022-12-07 21:35:56.523810192 +0100
> +++ gcc/testsuite/gcc.dg/pr106719.c     2022-12-07 21:35:40.964034788 +0100
> @@ -0,0 +1,13 @@
> +/* PR debug/106719 */
> +/* { dg-do compile { target sync_char_short } } */
> +/* { dg-options "-O2 -fcompare-debug" } */
> +
> +extern short int esi, easi[2];
> +
> +void
> +foo (void)
> +{
> +  short int *psi = &easi[1];
> +  __atomic_nand_fetch (psi, esi, 0);
> +  psi = &easi[1];
> +}
>
>         Jakub
>
  

Patch

--- gcc/cfgbuild.cc.jj	2022-01-18 11:58:58.944991171 +0100
+++ gcc/cfgbuild.cc	2022-12-07 21:36:27.493363173 +0100
@@ -445,6 +445,7 @@  find_bb_boundaries (basic_block bb)
   rtx_insn *debug_insn = NULL;
   edge fallthru = NULL;
   bool skip_purge;
+  bool seen_note_after_debug = false;
 
   if (insn == end)
     return;
@@ -492,7 +493,10 @@  find_bb_boundaries (basic_block bb)
       if (code == DEBUG_INSN)
 	{
 	  if (flow_transfer_insn && !debug_insn)
-	    debug_insn = insn;
+	    {
+	      debug_insn = insn;
+	      seen_note_after_debug = false;
+	    }
 	}
       /* In case we've previously seen an insn that effects a control
 	 flow transfer, split the block.  */
@@ -506,7 +510,40 @@  find_bb_boundaries (basic_block bb)
 	     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);
+	    {
+	      prev = PREV_INSN (debug_insn);
+	      if (seen_note_after_debug)
+		{
+		  /* Though, if there are NOTEs intermixed with DEBUG_INSNs,
+		     move the NOTEs before the DEBUG_INSNs and split after
+		     the last NOTE.  */
+		  rtx_insn *first = NULL, *last = NULL;
+		  for (x = debug_insn; x != insn; x = NEXT_INSN (x))
+		    {
+		      if (NOTE_P (x))
+			{
+			  if (first == NULL)
+			    first = x;
+			  last = x;
+			}
+		      else
+			{
+			  gcc_assert (DEBUG_INSN_P (x));
+			  if (first)
+			    {
+			      reorder_insns_nobb (first, last, prev);
+			      prev = last;
+			      first = last = NULL;
+			    }
+			}
+		    }
+		  if (first)
+		    {
+		      reorder_insns_nobb (first, last, prev);
+		      prev = last;
+		    }
+		}
+	    }
 	  fallthru = split_block (bb, prev);
 	  if (flow_transfer_insn)
 	    {
@@ -547,6 +584,14 @@  find_bb_boundaries (basic_block bb)
 	    flow_transfer_insn = prev_nonnote_nondebug_insn_bb (insn);
 	  debug_insn = NULL;
 	}
+      else if (debug_insn)
+	{
+	  if (code == NOTE)
+	    seen_note_after_debug = true;
+	  else
+	    /* Jump tables.  */
+	    debug_insn = NULL;
+	}
 
       if (control_flow_insn_p (insn))
 	flow_transfer_insn = insn;
--- gcc/testsuite/gcc.dg/pr106719.c.jj	2022-12-07 21:35:56.523810192 +0100
+++ gcc/testsuite/gcc.dg/pr106719.c	2022-12-07 21:35:40.964034788 +0100
@@ -0,0 +1,13 @@ 
+/* PR debug/106719 */
+/* { dg-do compile { target sync_char_short } } */
+/* { dg-options "-O2 -fcompare-debug" } */
+
+extern short int esi, easi[2];
+
+void
+foo (void)
+{
+  short int *psi = &easi[1];
+  __atomic_nand_fetch (psi, esi, 0);
+  psi = &easi[1];
+}