diff mbox

Fix PR45792, cris-elf build breakage from PR44374-fix "ifcvt/crossjump patch: Fix PR 42496, 21803"

Message ID 201009270911.o8R9B4BX011306@ignucius.se.axis.com
State New
Headers show

Commit Message

Hans-Peter Nilsson Sept. 27, 2010, 9:11 a.m. UTC 
(I'm replying to the recent message regarding the patch.)
The title was missing PR44374, I'm not sure whether there is
overlap or just a cutnpasto.

> Date: Thu, 23 Sep 2010 12:07:51 +0200
> From: Bernd Schmidt <bernds@codesourcery.com>

> Thanks.  Committed with a small fix found while testing on x86_64: In
> try_head_merge_bb, in the case where we try to move across multiple
> blocks to optimize for a switch statement, the final destination block
> must be the only predecessor of the block we're looking at.

Revision 164552 broke build for cris-elf, see PR45792.

Looking at the patch, I can't see how the "Try again"-codepath
(see patch below) can ever work; the retry will try merging the
insns starting with the NEXT_INSN after the old, merged, insns.
But, the insns are merged, so the old NEXT_INSN is lost; the new
NEXT_INSN is now move_before (the compare insn for the cbranch
sequence), so you'll try moving the compare and jump to before
the jump.  Which will cause reorder_insns to get stuck in a
loop, after having set PREV_INSN(cmp) = NEXT_INSN (cmp) = cmp...

How did that pass testing?  Is there something I (we both) miss
causing that code-path to never execute for non-cc0 targets?

Is the following what you had in mind?  I don't like adding
more #ifdef HAVE_cc0, but it's either there or in
can_move_insns_across, and the latter seems like the worse choice.
I'll test this native x86_64 and cross to cris-elf, but perhaps
you meant something different altogether so I thought better
give early notice.  Maybe reconsider and lose the loopness of
the do...while(!moveall) loop, as that apparently doesn't happen
very often, at least not successfully. :)

Perhaps a sanity-check for reorder_insns would is be in order.
(With it, I couldn't spot any slowdown for the .52s user time
that the test-case compiled, considering that reorder_insns_nobb
is now linear in the insn range length, but I didn't check
further.)

Ok to commit, either or all parts, after testing?

gcc/ChangeLog:
	PR rtl-optimization/45792
	* cfgcleanup.c (try_head_merge_bb): New rtx vector nextptr.
	If not all insns are to be merged, for each edge, stash the
	NEXT_INSNs after the to-be-merged insns before doing the merge,
	and use them for the retry at the new insertion point.

	* emit-rtl.c (reorder_insns_nobb) [ENABLE_CHECKING]: Sanity-check
	that AFTER is not in the range FROM..TO, inclusive.


brgds, H-P
diff mbox

Patch

--- gcc/cfgcleanup.c~	Sun Sep 26 08:24:42 2010
+++ gcc/cfgcleanup.c	Mon Sep 27 07:33:36 2010
@@ -1944,7 +1941,7 @@  try_head_merge_bb (basic_block bb)
   basic_block final_dest_bb = NULL;
   int max_match = INT_MAX;
   edge e0;
-  rtx *headptr, *currptr;
+  rtx *headptr, *currptr, *nextptr;
   bool changed, moveall;
   unsigned ix;
   rtx e0_last_head, cond, move_before;
@@ -2077,6 +2074,7 @@  try_head_merge_bb (basic_block bb)
 
   currptr = XNEWVEC (rtx, nedges);
   headptr = XNEWVEC (rtx, nedges);
+  nextptr = XNEWVEC (rtx, nedges);
 
   for (ix = 0; ix < nedges; ix++)
     {
@@ -2132,6 +2130,14 @@  try_head_merge_bb (basic_block bb)
 
 	  /* Try again, using a different insertion point.  */
 	  move_before = jump;
+
+#ifdef HAVE_cc0
+	  /* Don't try moving before a cc0 user, as that may invalidate
+	     the cc0.  */
+	  if (reg_mentioned_p (cc0_rtx, jump))
+	    break;
+#endif
+
 	  continue;
 	}
 
@@ -2155,6 +2161,12 @@  try_head_merge_bb (basic_block bb)
 	    }
 	}
 
+      /* If we can't currently move all of the identical insns, remember
+	 each insn after the range that we'll merge.  */
+      if (!moveall)
+	for (ix = 0; ix < nedges; ix++)
+	  nextptr[ix] = NEXT_INSN (currptr[ix]);
+
       reorder_insns (headptr[0], currptr[0], PREV_INSN (move_before));
       df_set_bb_dirty (EDGE_SUCC (bb, 0)->dest);
       if (final_dest_bb != NULL)
@@ -2170,14 +2182,21 @@  try_head_merge_bb (basic_block bb)
 	  if (jump == move_before)
 	    break;
 
-	  /* Try again, using a different insertion point.  */
+	  /* For the unmerged insns, try a different insertion point.  */
 	  move_before = jump;
+
+#ifdef HAVE_cc0
+	  /* Don't try moving before a cc0 user, as that may invalidate
+	     the cc0.  */
+	  if (reg_mentioned_p (cc0_rtx, jump))
+	    break;
+#endif
+
 	  for (ix = 0; ix < nedges; ix++)
 	    {
-	      rtx curr = currptr[ix];
-	      do
-		curr = NEXT_INSN (curr);
+	      rtx curr = nextptr[ix];
 	      while (!NONDEBUG_INSN_P (curr));
+		curr = NEXT_INSN (curr);
 	      currptr[ix] = headptr[ix] = curr;
 	    }
 	}
--- gcc/emit-rtl.c~	Sun Sep 26 08:24:42 2010
+++ gcc/emit-rtl.c	Mon Sep 27 08:19:20 2010
@@ -3972,16 +3972,23 @@  delete_insns_since (rtx from)
    AFTER must not be FROM or TO or any insn in between.
 
    This function does not know about SEQUENCEs and hence should not be
    called after delay-slot filling has been done.  */
 
 void
 reorder_insns_nobb (rtx from, rtx to, rtx after)
 {
+#ifdef ENABLE_CHECKING
+  rtx x;
+  for (x = from; x != to; x = NEXT_INSN (x))
+    gcc_assert (after != x);
+  gcc_assert (after != to);
+#endif
+
   /* Splice this bunch out of where it is now.  */
   if (PREV_INSN (from))
     NEXT_INSN (PREV_INSN (from)) = NEXT_INSN (to);
   if (NEXT_INSN (to))
     PREV_INSN (NEXT_INSN (to)) = PREV_INSN (from);
   if (get_last_insn () == to)
     set_last_insn (PREV_INSN (from));
   if (get_insns () == from)