dbr_schedule vs uninitialized registers (2)

dbr_schedule vs uninitialized registers (2)

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

More than one year ago Richard Sandiford diagnosed a problem with the liveness 
computation of the delayed branches scheduling pass and proposed a patch:
  http://gcc.gnu.org/ml/gcc-patches/2007-12/msg00506.html

I suggested to use a simpler approach instead (changing the DF problem to LR 
to expose uninitialized registers) and this was sufficient to fix the bug.

Ironically enough, there is a problem on the SPARC with this approach because 
of the save register window instruction: it clobbers all the registers on 
function entry so uninitialized registers are not detected as live if there 
is no BARRIER between the entry and the instruction.  That's pretty rare in 
practice but I have an Ada testcase.

That's why I've finally applied Richard's patch on the mainline as well as 
changed back the DF problem to LIVE.  Tested on sparc-sun-solaris2.9 and 
sparc64-sun-solaris2.10.


2009-04-27  Richard Sandiford  <rdsandiford@googlemail.com>
            Eric Botcazou  <ebotcazou@adacore.com>

	* resource.c (find_basic_block): Use BLOCK_FOR_INSN to look up
	a label's basic block.
	(mark_target_live_regs): Tidy and rework obsolete comments.
	Change back DF problem to LIVE.  If a label starts a basic block,
	assume that all registers that used to be live then still are.
	(init_resource_info): If a label starts a basic block, set its
	BLOCK_FOR_INSN accordingly.
	(free_resource_info): Undo the setting of BLOCK_FOR_INSN.


2009-04-27  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt2.adb: New test.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 4847 bytes --]

Index: resource.c
===================================================================
--- resource.c	(revision 146825)
+++ resource.c	(working copy)
@@ -135,8 +135,6 @@ update_live_status (rtx dest, const_rtx 
 static int
 find_basic_block (rtx insn, int search_limit)
 {
-  basic_block bb;
-
   /* Scan backwards to the previous BARRIER.  Then see if we can find a
      label that starts a basic block.  Return the basic block number.  */
   for (insn = prev_nonnote_insn (insn);
@@ -157,11 +155,8 @@ find_basic_block (rtx insn, int search_l
   for (insn = next_nonnote_insn (insn);
        insn && LABEL_P (insn);
        insn = next_nonnote_insn (insn))
-    {
-      FOR_EACH_BB (bb)
-	if (insn == BB_HEAD (bb))
-	  return bb->index;
-    }
+    if (BLOCK_FOR_INSN (insn))
+      return BLOCK_FOR_INSN (insn)->index;
 
   return -1;
 }
@@ -848,13 +843,12 @@ return_insn_p (const_rtx insn)
    (with no intervening active insns) to see if any of them start a basic
    block.  If we hit the start of the function first, we use block 0.
 
-   Once we have found a basic block and a corresponding first insns, we can
-   accurately compute the live status from basic_block_live_regs and
-   reg_renumber.  (By starting at a label following a BARRIER, we are immune
-   to actions taken by reload and jump.)  Then we scan all insns between
-   that point and our target.  For each CLOBBER (or for call-clobbered regs
-   when we pass a CALL_INSN), mark the appropriate registers are dead.  For
-   a SET, mark them as live.
+   Once we have found a basic block and a corresponding first insn, we can
+   accurately compute the live status (by starting at a label following a
+   BARRIER, we are immune to actions taken by reload and jump.)  Then we
+   scan all insns between that point and our target.  For each CLOBBER (or
+   for call-clobbered regs when we pass a CALL_INSN), mark the appropriate
+   registers are dead.  For a SET, mark them as live.
 
    We have to be careful when using REG_DEAD notes because they are not
    updated by such things as find_equiv_reg.  So keep track of registers
@@ -954,13 +948,10 @@ mark_target_live_regs (rtx insns, rtx ta
      TARGET.  Otherwise, we must assume everything is live.  */
   if (b != -1)
     {
-      regset regs_live = DF_LR_IN (BASIC_BLOCK (b));
+      regset regs_live = df_get_live_in (BASIC_BLOCK (b));
       rtx start_insn, stop_insn;
 
-      /* Compute hard regs live at start of block -- this is the real hard regs
-	 marked live, plus live pseudo regs that have been renumbered to
-	 hard regs.  */
-
+      /* Compute hard regs live at start of block.  */
       REG_SET_TO_HARD_REG_SET (current_live_regs, regs_live);
 
       /* Get starting and ending insn, handling the case where each might
@@ -1046,10 +1037,24 @@ mark_target_live_regs (rtx insns, rtx ta
 
 	  else if (LABEL_P (real_insn))
 	    {
+	      basic_block bb;
+
 	      /* A label clobbers the pending dead registers since neither
 		 reload nor jump will propagate a value across a label.  */
 	      AND_COMPL_HARD_REG_SET (current_live_regs, pending_dead_regs);
 	      CLEAR_HARD_REG_SET (pending_dead_regs);
+
+	      /* We must conservatively assume that all registers that used
+		 to be live here still are.  The fallthrough edge may have
+		 left a live register uninitialized.  */
+	      bb = BLOCK_FOR_INSN (real_insn);
+	      if (bb)
+		{
+		  HARD_REG_SET extra_live;
+
+		  REG_SET_TO_HARD_REG_SET (extra_live, df_get_live_in (bb));
+		  IOR_HARD_REG_SET (current_live_regs, extra_live);
+		}
 	    }
 
 	  /* The beginning of the epilogue corresponds to the end of the
@@ -1121,6 +1126,7 @@ void
 init_resource_info (rtx epilogue_insn)
 {
   int i;
+  basic_block bb;
 
   /* Indicate what resources are required to be valid at the end of the current
      function.  The condition code never is and memory always is.  If the
@@ -1189,6 +1195,11 @@ init_resource_info (rtx epilogue_insn)
   /* Allocate and initialize the tables used by mark_target_live_regs.  */
   target_hash_table = XCNEWVEC (struct target_info *, TARGET_HASH_PRIME);
   bb_ticks = XCNEWVEC (int, last_basic_block);
+
+  /* Set the BLOCK_FOR_INSN of each label that starts a basic block.  */
+  FOR_EACH_BB (bb)
+    if (LABEL_P (BB_HEAD (bb)))
+      BLOCK_FOR_INSN (BB_HEAD (bb)) = bb;
 }
 \f
 /* Free up the resources allocated to mark_target_live_regs ().  This
@@ -1197,6 +1208,8 @@ init_resource_info (rtx epilogue_insn)
 void
 free_resource_info (void)
 {
+  basic_block bb;
+
   if (target_hash_table != NULL)
     {
       int i;
@@ -1222,6 +1235,10 @@ free_resource_info (void)
       free (bb_ticks);
       bb_ticks = NULL;
     }
+
+  FOR_EACH_BB (bb)
+    if (LABEL_P (BB_HEAD (bb)))
+      BLOCK_FOR_INSN (BB_HEAD (bb)) = NULL;
 }
 \f
 /* Clear any hashed information that we have stored for INSN.  */

[-- Attachment #3: opt2.adb --]
[-- Type: text/x-adasrc, Size: 564 bytes --]

-- { dg-do run }
-- { dg-options "-O2 -fno-inline" }

procedure Opt2 is
   function Get return String is
   begin
      return "[]";
   end Get;

   Message : String := Get;

   F, L : Integer;
begin
   for J in Message'Range loop
      if Message (J) = '[' then
         F := J;
      elsif Message (J) = ']' then
         L := J;
         exit;
      end if;
   end loop;

   declare
      M : String :=
         Message (Message'First .. F) & Message (L .. Message'Last);
   begin
      if M /= "[]" then
        raise Program_Error;
      end if;
   end;
end;

Re: dbr_schedule vs uninitialized registers (2)

[Hans-Peter Nilsson]

On Mon, 27 Apr 2009, Eric Botcazou wrote:
> 2009-04-27  Richard Sandiford  <rdsandiford@googlemail.com>
>             Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* resource.c (find_basic_block): Use BLOCK_FOR_INSN to look up
> 	a label's basic block.
> 	(mark_target_live_regs): Tidy and rework obsolete comments.
> 	Change back DF problem to LIVE.  If a label starts a basic block,
> 	assume that all registers that used to be live then still are.
> 	(init_resource_info): If a label starts a basic block, set its
> 	BLOCK_FOR_INSN accordingly.
> 	(free_resource_info): Undo the setting of BLOCK_FOR_INSN.

This caused PR39938.

brgds, H-P