Re: Fill more delay slots in conditional returns

[Eric Botcazou <ebotcazou at adacore dot com>]

> I don't recall ever working on this aspect of reorg.  The obvious worry
> is that with reorg moving stuff around those notes may not be valid
> anymore in the general case.

Yes, in the general case I agree that's too dangerous.  In this particular 
case, i.e. backward scan only, this might be plausible, although one has 
probably to worry about what happens if the insn is removed from the delay 
slot and put back into the RTL stream.

> Just a note, there's a formatting error in this code.  The real_insn =
> statement is indented too far.  Feel free to fix that :-)

Patch attached (I've also reindented a block of code in reorg.c), tested on 
the SPARC and applied on the mainline.

> Seems reasonable.  This might just be an oversight by the original
> author.  Effectively we're going to start marking the referenced
> resources with your patch.  We still defer killing as far as I can tell.

OK.

> If you want to run with it, I won't object.  My worry would be we might
> not see the fallout for a long time as the number of things that have to
> fall into place for an observable failure here are very high.

Indeed, I spotted these two cases when I was playing with the allocation of 
registers for the private target.  They showed up only under very specific 
circumstances; with the default setting, neither makes a difference for the 
various testcases I'm looking at.  So, in the end, I probably won't pursue.

Thanks for your feedback.


	* reorg.c (fill_simple_delay_slots): Reindent block of code.
	* resource.c (mark_target_live_regs): Reformat conditional block.


-- 
Eric Botcazou

Index: reorg.c
===================================================================
--- reorg.c	(revision 197926)
+++ reorg.c	(working copy)
@@ -2144,69 +2144,66 @@ fill_simple_delay_slots (int non_jumps_p
 	  if (CALL_P (insn))
 	    maybe_never = 1;
 
-	    for (trial = next_nonnote_insn (insn); !stop_search_p (trial, 1);
-		 trial = next_trial)
-	      {
-		next_trial = next_nonnote_insn (trial);
-
-		/* This must be an INSN or CALL_INSN.  */
-		pat = PATTERN (trial);
-
-		/* Stand-alone USE and CLOBBER are just for flow.  */
-		if (GET_CODE (pat) == USE || GET_CODE (pat) == CLOBBER)
-		  continue;
-
-		/* If this already has filled delay slots, get the insn needing
-		   the delay slots.  */
-		if (GET_CODE (pat) == SEQUENCE)
-		  trial_delay = XVECEXP (pat, 0, 0);
-		else
-		  trial_delay = trial;
-
-		/* Stop our search when seeing a jump.  */
-		if (JUMP_P (trial_delay))
-		  break;
-
-		/* See if we have a resource problem before we try to
-		   split.  */
-		if (GET_CODE (pat) != SEQUENCE
-		    && ! insn_references_resource_p (trial, &set, true)
-		    && ! insn_sets_resource_p (trial, &set, true)
-		    && ! insn_sets_resource_p (trial, &needed, true)
+	  for (trial = next_nonnote_insn (insn); !stop_search_p (trial, 1);
+	       trial = next_trial)
+	    {
+	      next_trial = next_nonnote_insn (trial);
+
+	      /* This must be an INSN or CALL_INSN.  */
+	      pat = PATTERN (trial);
+
+	      /* Stand-alone USE and CLOBBER are just for flow.  */
+	      if (GET_CODE (pat) == USE || GET_CODE (pat) == CLOBBER)
+		continue;
+
+	      /* If this already has filled delay slots, get the insn needing
+		 the delay slots.  */
+	      if (GET_CODE (pat) == SEQUENCE)
+		trial_delay = XVECEXP (pat, 0, 0);
+	      else
+		trial_delay = trial;
+
+	      /* Stop our search when seeing a jump.  */
+	      if (JUMP_P (trial_delay))
+		break;
+
+	      /* See if we have a resource problem before we try to split.  */
+	      if (GET_CODE (pat) != SEQUENCE
+		  && ! insn_references_resource_p (trial, &set, true)
+		  && ! insn_sets_resource_p (trial, &set, true)
+		  && ! insn_sets_resource_p (trial, &needed, true)
 #ifdef HAVE_cc0
-		    && ! (reg_mentioned_p (cc0_rtx, pat) && ! sets_cc0_p (pat))
+		  && ! (reg_mentioned_p (cc0_rtx, pat) && ! sets_cc0_p (pat))
 #endif
-		    && ! (maybe_never && may_trap_or_fault_p (pat))
-		    && (trial = try_split (pat, trial, 0))
-		    && eligible_for_delay (insn, slots_filled, trial, flags)
-		    && ! can_throw_internal(trial))
-		  {
-		    next_trial = next_nonnote_insn (trial);
-		    delay_list = add_to_delay_list (trial, delay_list);
-
+		  && ! (maybe_never && may_trap_or_fault_p (pat))
+		  && (trial = try_split (pat, trial, 0))
+		  && eligible_for_delay (insn, slots_filled, trial, flags)
+		  && ! can_throw_internal(trial))
+		{
+		  next_trial = next_nonnote_insn (trial);
+		  delay_list = add_to_delay_list (trial, delay_list);
 #ifdef HAVE_cc0
-		    if (reg_mentioned_p (cc0_rtx, pat))
-		      link_cc0_insns (trial);
+		  if (reg_mentioned_p (cc0_rtx, pat))
+		    link_cc0_insns (trial);
 #endif
+		  delete_related_insns (trial);
+		  if (slots_to_fill == ++slots_filled)
+		    break;
+		  continue;
+		}
+
+	      mark_set_resources (trial, &set, 0, MARK_SRC_DEST_CALL);
+	      mark_referenced_resources (trial, &needed, true);
 
-		    delete_related_insns (trial);
-		    if (slots_to_fill == ++slots_filled)
-		      break;
-		    continue;
-		  }
-
-		mark_set_resources (trial, &set, 0, MARK_SRC_DEST_CALL);
-		mark_referenced_resources (trial, &needed, true);
-
-		/* Ensure we don't put insns between the setting of cc and the
-		   comparison by moving a setting of cc into an earlier delay
-		   slot since these insns could clobber the condition code.  */
-		set.cc = 1;
-
-		/* If this is a call, we might not get here.  */
-		if (CALL_P (trial_delay))
-		  maybe_never = 1;
-	      }
+	      /* Ensure we don't put insns between the setting of cc and the
+		 comparison by moving a setting of cc into an earlier delay
+		 slot since these insns could clobber the condition code.  */
+	      set.cc = 1;
+
+	      /* If this is a call, we might not get here.  */
+	      if (CALL_P (trial_delay))
+		maybe_never = 1;
+	    }
 
 	  /* If there are slots left to fill and our search was stopped by an
 	     unconditional branch, try the insn at the branch target.  We can
Index: resource.c
===================================================================
--- resource.c	(revision 197926)
+++ resource.c	(working copy)
@@ -990,9 +990,10 @@ mark_target_live_regs (rtx insns, rtx ta
 
 	  /* If this insn is a USE made by update_block, we care about the
 	     underlying insn.  */
-	  if (code == INSN && GET_CODE (PATTERN (insn)) == USE
+	  if (code == INSN
+	      && GET_CODE (PATTERN (insn)) == USE
 	      && INSN_P (XEXP (PATTERN (insn), 0)))
-	      real_insn = XEXP (PATTERN (insn), 0);
+	    real_insn = XEXP (PATTERN (insn), 0);
 
 	  if (CALL_P (real_insn))
 	    {