diff mbox

Fix mark_all_labels vs cfglayout mode

Message ID 4E2A0C16.30700@redhat.com
State New
Headers show

Commit Message

Richard Henderson July 22, 2011, 11:47 p.m. UTC
I re-tested my dwarf2 cfi propagation pass today, with the 
partitioning vs eh edge splitting fix included.  Only this
time I tested languages=all,ada,go instead of just =c,c++.
Lo and behold I find another latent problem, this time via
a Fortran test case.

The dwarf2 cfg pass is set to abort for unreachable blocks,
and gfortran.dg/debug/pr46756.f found such a case.

The test case contains a computed goto.  Except that during
combine, we (incorrectly) filled in the JUMP_LABEL of that
jump insn.  This, by definition, means it is not computed_jump_p.
Which means that when it came to pass_dwarf2, several blocks
that ought to have been reachable via forced_labels weren't.

During normal optimization we don't really care about JUMP_LABEL
because we rely on the CFG.  And I guess nothing else really
cared either, since somehow the test case worked.  We should
probably have a check for this in rtl_verify_flow_info...

The incorrect substitution in combine happened because
mark_all_labels scans extended basic blocks the old skool way,
by looking for labels.  Except that in cfglayout mode we
don't bother with unimportant bits like labels, so m_a_l got
the wrong answer.

If we admit that something as simplistic as constant propagation
between adjacent instructions is something that's actually useful
in the specific context of being in cfglayout mode while lots of
other optimizers are running, then, really, there's other work 
that should be done when this substitution is made.  Namely, clean
up the dead CFG edges.

However, I do not in fact agree with the preceding predicate.
And a bootstrap and test run with a well-placed assertion agree:
this is not something that ever happens.

Which makes the resulting code much tidier.

Tested on x86_64-linux.  Committed.


r~
* jump.c (maybe_propagate_label_ref): Split out of...
        (mark_all_labels): ... here.  Do not attempt label_ref
        propagation while in cfglayout mode.
diff mbox

Patch

diff --git a/gcc/jump.c b/gcc/jump.c
index f337eb3..0d29f0f 100644
--- a/gcc/jump.c
+++ b/gcc/jump.c
@@ -200,6 +200,54 @@  init_label_info (rtx f)
     }
 }
 
+/* A subroutine of mark_all_labels.  Trivially propagate a simple label
+   load into a jump_insn that uses it.  */
+
+static void
+maybe_propagate_label_ref (rtx jump_insn, rtx prev_nonjump_insn)
+{
+  rtx label_note, pc, pc_src;
+
+  pc = pc_set (jump_insn);
+  pc_src = pc != NULL ? SET_SRC (pc) : NULL;
+  label_note = find_reg_note (prev_nonjump_insn, REG_LABEL_OPERAND, NULL);
+
+  /* If the previous non-jump insn sets something to a label,
+     something that this jump insn uses, make that label the primary
+     target of this insn if we don't yet have any.  That previous
+     insn must be a single_set and not refer to more than one label.
+     The jump insn must not refer to other labels as jump targets
+     and must be a plain (set (pc) ...), maybe in a parallel, and
+     may refer to the item being set only directly or as one of the
+     arms in an IF_THEN_ELSE.  */
+
+  if (label_note != NULL && pc_src != NULL)
+    {
+      rtx label_set = single_set (prev_nonjump_insn);
+      rtx label_dest = label_set != NULL ? SET_DEST (label_set) : NULL;
+
+      if (label_set != NULL
+	  /* The source must be the direct LABEL_REF, not a
+	     PLUS, UNSPEC, IF_THEN_ELSE etc.  */
+	  && GET_CODE (SET_SRC (label_set)) == LABEL_REF
+	  && (rtx_equal_p (label_dest, pc_src)
+	      || (GET_CODE (pc_src) == IF_THEN_ELSE
+		  && (rtx_equal_p (label_dest, XEXP (pc_src, 1))
+		      || rtx_equal_p (label_dest, XEXP (pc_src, 2))))))
+	{
+	  /* The CODE_LABEL referred to in the note must be the
+	     CODE_LABEL in the LABEL_REF of the "set".  We can
+	     conveniently use it for the marker function, which
+	     requires a LABEL_REF wrapping.  */
+	  gcc_assert (XEXP (label_note, 0) == XEXP (SET_SRC (label_set), 0));
+
+	  mark_jump_label_1 (label_set, jump_insn, false, true);
+
+	  gcc_assert (JUMP_LABEL (jump_insn) == XEXP (label_note, 0));
+	}
+    }
+}
+
 /* Mark the label each jump jumps to.
    Combine consecutive labels, and count uses of labels.  */
 
@@ -207,85 +255,31 @@  static void
 mark_all_labels (rtx f)
 {
   rtx insn;
-  rtx prev_nonjump_insn = NULL;
-
-  for (insn = f; insn; insn = NEXT_INSN (insn))
-    if (NONDEBUG_INSN_P (insn))
-      {
-	mark_jump_label (PATTERN (insn), insn, 0);
-
-	/* If the previous non-jump insn sets something to a label,
-	   something that this jump insn uses, make that label the primary
-	   target of this insn if we don't yet have any.  That previous
-	   insn must be a single_set and not refer to more than one label.
-	   The jump insn must not refer to other labels as jump targets
-	   and must be a plain (set (pc) ...), maybe in a parallel, and
-	   may refer to the item being set only directly or as one of the
-	   arms in an IF_THEN_ELSE.  */
-	if (! INSN_DELETED_P (insn)
-	    && JUMP_P (insn)
-	    && JUMP_LABEL (insn) == NULL)
-	  {
-	    rtx label_note = NULL;
-	    rtx pc = pc_set (insn);
-	    rtx pc_src = pc != NULL ? SET_SRC (pc) : NULL;
-
-	    if (prev_nonjump_insn != NULL)
-	      label_note
-		= find_reg_note (prev_nonjump_insn, REG_LABEL_OPERAND, NULL);
-
-	    if (label_note != NULL && pc_src != NULL)
-	      {
-		rtx label_set = single_set (prev_nonjump_insn);
-		rtx label_dest
-		  = label_set != NULL ? SET_DEST (label_set) : NULL;
-
-		if (label_set != NULL
-		    /* The source must be the direct LABEL_REF, not a
-		       PLUS, UNSPEC, IF_THEN_ELSE etc.  */
-		    && GET_CODE (SET_SRC (label_set)) == LABEL_REF
-		    && (rtx_equal_p (label_dest, pc_src)
-			|| (GET_CODE (pc_src) == IF_THEN_ELSE
-			    && (rtx_equal_p (label_dest, XEXP (pc_src, 1))
-				|| rtx_equal_p (label_dest,
-						XEXP (pc_src, 2))))))
-
-		  {
-		    /* The CODE_LABEL referred to in the note must be the
-		       CODE_LABEL in the LABEL_REF of the "set".  We can
-		       conveniently use it for the marker function, which
-		       requires a LABEL_REF wrapping.  */
-		    gcc_assert (XEXP (label_note, 0)
-				== XEXP (SET_SRC (label_set), 0));
-
-		    mark_jump_label_1 (label_set, insn, false, true);
-		    gcc_assert (JUMP_LABEL (insn)
-				== XEXP (SET_SRC (label_set), 0));
-		  }
-	      }
-	  }
-	else if (! INSN_DELETED_P (insn))
-	  prev_nonjump_insn = insn;
-      }
-    else if (LABEL_P (insn))
-      prev_nonjump_insn = NULL;
 
-  /* If we are in cfglayout mode, there may be non-insns between the
-     basic blocks.  If those non-insns represent tablejump data, they
-     contain label references that we must record.  */
   if (current_ir_type () == IR_RTL_CFGLAYOUT)
     {
       basic_block bb;
-      rtx insn;
       FOR_EACH_BB (bb)
 	{
+	  /* In cfglayout mode, we don't bother with trivial next-insn
+	     propagation of LABEL_REFs into JUMP_LABEL.  This will be
+	     handled by other optimizers using better algorithms.  */
+	  FOR_BB_INSNS (bb, insn)
+	    {
+	      gcc_assert (! INSN_DELETED_P (insn));
+	      if (NONDEBUG_INSN_P (insn))
+	        mark_jump_label (PATTERN (insn), insn, 0);
+	    }
+
+	  /* In cfglayout mode, there may be non-insns between the
+	     basic blocks.  If those non-insns represent tablejump data,
+	     they contain label references that we must record.  */
 	  for (insn = bb->il.rtl->header; insn; insn = NEXT_INSN (insn))
 	    if (INSN_P (insn))
 	      {
 		gcc_assert (JUMP_TABLE_DATA_P (insn));
 		mark_jump_label (PATTERN (insn), insn, 0);
 	      }
-
 	  for (insn = bb->il.rtl->footer; insn; insn = NEXT_INSN (insn))
 	    if (INSN_P (insn))
 	      {
@@ -294,6 +288,28 @@  mark_all_labels (rtx f)
 	      }
 	}
     }
+  else
+    {
+      rtx prev_nonjump_insn = NULL;
+      for (insn = f; insn; insn = NEXT_INSN (insn))
+	{
+	  if (INSN_DELETED_P (insn))
+	    ;
+	  else if (LABEL_P (insn))
+	    prev_nonjump_insn = NULL;
+	  else if (NONDEBUG_INSN_P (insn))
+	    {
+	      mark_jump_label (PATTERN (insn), insn, 0);
+	      if (JUMP_P (insn))
+		{
+		  if (JUMP_LABEL (insn) == NULL && prev_nonjump_insn != NULL)
+		    maybe_propagate_label_ref (insn, prev_nonjump_insn);
+		}
+	      else
+		prev_nonjump_insn = insn;
+	    }
+	}
+    }
 }
 
 /* Given a comparison (CODE ARG0 ARG1), inside an insn, INSN, return a code