diff mbox

RFA: Fix dse / postreload not to bypass add expanders

Message ID 20111101045348.qp9lxw7jhkogcook-nzlynne@webmail.spamcop.net
State New
Headers show

Commit Message

Joern Rennecke Nov. 1, 2011, 8:53 a.m. UTC 
This patch makes emit_inc_dec_insn_before use add3_insn / gen_move_insn
so that the appropriate expanders are used to create the new instructions,
and for dse it use the available register liveness information to check
that no live fixed hard register, like a flags register, is clobbered in the
process.  For postreload, there is no such information available, so we
give up when we see a clobber / set that might be problematic.

regtested for epiphany-elf with modified rtx_cost, where it fixes  
three ICE-on-valid-code:
FAIL: gcc.c-torture/execute/builtins/memcpy-chk.c compilation,  -O1   
(internal compiler error)
FAIL: gcc.c-torture/execute/builtins/memmove-chk.c compilation,  -O1   
(internal compiler error)
FAIL: gcc.c-torture/execute/memcpy-bi.c compilation,  -O1  (internal  
compiler error)

Bootstrapped and regression tested on i686-pc-linux-gnu .
2011-10-31  Joern Rennecke <joern.rennecke@embecosm.com>

	* regset.h (fixed_regset): Declare.
	* dse.c: Include regset.h .
	(struct insn_info): Add member fixed_regs_live.
	(note_add_store_info): New typedef.
	(note_add_store): New function.
	(emit_inc_dec_insn_before): Expect arg to be of type insn_info_t .
	Use gen_add3_insn / gen_move_insn.
	Check new insn for unwanted clobbers before emitting it.
	(check_for_inc_dec): Rename to...
	(check_for_inc_dec_1:) ... this.  Return bool.  Take insn_info
	parameter.  Changed all callers in file.
	(check_for_inc_dec, copy_fixed_regs): New functions.
	(scan_insn): Set fixed_regs_live field of insn_info.
	* rtl.h (check_for_inc_dec): Update prototype.
	* postreload.c (reload_cse_simplify): Take new signature of
	check_ind_dec into account.
	* reginfo.c (fixed_regset): New variable.
	(init_reg_sets_1): Initialize it.

Comments

Eric Botcazou Nov. 3, 2011, 7:01 p.m. UTC | #1 
> This patch makes emit_inc_dec_insn_before use add3_insn / gen_move_insn
> so that the appropriate expanders are used to create the new instructions,
> and for dse it use the available register liveness information to check
> that no live fixed hard register, like a flags register, is clobbered in
> the process.  For postreload, there is no such information available, so we
> give up when we see a clobber / set that might be problematic.

2011-10-31  Joern Rennecke <joern.rennecke@embecosm.com>

	* regset.h (fixed_regset): Declare.
	* dse.c: Include regset.h .
	(struct insn_info): Add member fixed_regs_live.
	(note_add_store_info): New typedef.
	(note_add_store): New function.
	(emit_inc_dec_insn_before): Expect arg to be of type insn_info_t .
	Use gen_add3_insn / gen_move_insn.
	Check new insn for unwanted clobbers before emitting it.
	(check_for_inc_dec): Rename to...
	(check_for_inc_dec_1:) ... this.  Return bool.  Take insn_info
	parameter.  Changed all callers in file.
	(check_for_inc_dec, copy_fixed_regs): New functions.
	(scan_insn): Set fixed_regs_live field of insn_info.
	* rtl.h (check_for_inc_dec): Update prototype.
	* postreload.c (reload_cse_simplify): Take new signature of
	check_ind_dec into account.
	* reginfo.c (fixed_regset): New variable.
	(init_reg_sets_1): Initialize it.

OK modulo the following:

+typedef struct
+{
+  rtx insert_before;

This field is never read.


+  rtx first, current;
+  regset fixed_regs_live;
+  bool failure;
+} note_add_store_info;
+
+/* Callback for emit_inc_dec_insn_before via note_stores.
+   Check if a register is clobbered which is life afterwards.  */

"live"


+static void
+note_add_store (rtx loc, const_rtx expr ATTRIBUTE_UNUSED, void *data)

Missing blank line.  The functions in dse.c have a blank line between head 
comment and body.


+{
+  rtx insn, *nextp;
+  note_add_store_info *info = (note_add_store_info *) data;
+  int r, n;
+
+  if (!REG_P (loc))
+    return;
+  /* If this register is referenced by the current or an earlier insn,
+     that's OK.  E.g. this applies to the register that is being incremented
+     with this addition.  */

Blank line before the comment.


+  nextp = &info->first;
+  do
+    {
+      insn = *nextp;
+      nextp = &NEXT_INSN (insn);
+      if (reg_referenced_p (loc, PATTERN (insn)))
+	return;
+    }
+  while (insn != info->current);

Isn't that a convoluted way of writing this?

  for (insn = info->first;
       insn != NEXT_INSN (info->current);
       insn = NEXT_INSN (insn))
    if (reg_referenced_p (loc, PATTERN (insn)))
      return;


+  if (!info->fixed_regs_live)
+    {
+      info->failure =  true;
+      return;
+    }

Missing comment explaining why we do that.



+  /* Now check if this is a live fixed register.  */
+  r = REGNO (loc);
+  n = HARD_REGNO_NREGS (r, GET_MODE (loc));
+  while (--n >=  0)
+    if (REGNO_REG_SET_P (info->fixed_regs_live, r+n))
+      info->failure =  true;

Blank line before the comment.  What's the point in the reverse iteration?

  for (i = 0; i < hard_regno_nregs[regno][GET_MODE (loc)]; i++)
    if (REGNO_REG_SET_P (info->fixed_regs_live, regno + i))
       {
         info->failure =  true;
         return;
       }

hard_regno_nregs in small letters.


+  info.insert_before = insn;
+  info.first = new_insn;
+  info.fixed_regs_live = insn_info->fixed_regs_live;
+  info.failure = false;
+  for (cur = new_insn; cur; cur = NEXT_INSN (cur))
+    {
+      info.current = cur;
+      note_stores (PATTERN (cur), note_add_store, &info);
+    }
+  if (info.failure)
+    return 1;

Missing comment explaining what we're doing.



 /* Before we delete INSN, make sure that the auto inc/dec, if it is
-   there, is split into a separate insn.  */
+   there, is split into a separate insn.
+   Return true on success (or if there was nothing to do), false on failure.  
*/
 
-void
-check_for_inc_dec (rtx insn)
+static bool
+check_for_inc_dec_1 (insn_info_t insn_info)

Missing adjustment in the comment: "Before we delete the insn described by
INSN_INFO, make sure..."


+/* Entry point for postreload.  */
+bool
+check_for_inc_dec (rtx insn, regset fixed_regs_live)

No point in adding an argument if it is always null.  Missing blank line and 
head comment: "Same as above, but take a naked INSN instead.  This is used by 
passes like that don't compute precise liveness information."


+/* Return a bitmap of the fixed registers contained in IN.  */
+static bitmap
+copy_fixed_regs (const_bitmap in)
+{
+  bitmap ret;
+
+  ret = ALLOC_REG_SET (NULL);
+  bitmap_and (ret, in, fixed_regset);
+  return ret;
+}

Missing blank line.


+/* Same information as fixed_reg_set but in regset form.  */
+regset fixed_regset;

Hum, you'd better have a good trick to remember which is which.  This isn't 
pretty, but let's mimic what is just above:

/* Same information as FIXED_REG_SET but in regset form.  */
regset fixed_reg_set_regset;
Paolo Bonzini Nov. 4, 2011, 10:10 a.m. UTC | #2 
On 11/03/2011 08:01 PM, Eric Botcazou wrote:
> +  info.insert_before = insn;
> +  info.first = new_insn;
> +  info.fixed_regs_live = insn_info->fixed_regs_live;
> +  info.failure = false;
> +  for (cur = new_insn; cur; cur = NEXT_INSN (cur))
> +    {
> +      info.current = cur;
> +      note_stores (PATTERN (cur), note_add_store,&info);
> +    }

Unless I'm missing something, this is going all the way down to the end 
of the function, bypassing the CFG, so it is neither efficient nor correct.

For DSE you should set up backwards liveness simulation, and use that 
instead of note_stores and insn scanning.  I don't know postreload well 
enough, but liveness simulation might work there too.

Paolo
Eric Botcazou Nov. 4, 2011, 10:24 a.m. UTC | #3 
> Unless I'm missing something, this is going all the way down to the end
> of the function, bypassing the CFG, so it is neither efficient nor correct.

No, gen_ functions doesn't emit the instructions.

> For DSE you should set up backwards liveness simulation, and use that
> instead of note_stores and insn scanning.

DSE already does this sort of insn scanning.
Joern Rennecke Nov. 4, 2011, 10:38 a.m. UTC | #4 
Quoting Paolo Bonzini <bonzini@gnu.org>:

> Unless I'm missing something, this is going all the way down to the end
> of the function, bypassing the CFG, so it is neither efficient nor
> correct.

new_insn hasn't been emitted yet, hence it is a single insn or a short
chain of insn to implement a no-op move (for a post-modify with zero offset)
or, more likely, a two-address add.
diff mbox

Patch

Index: postreload.c
===================================================================
--- postreload.c	(revision 180683)
+++ postreload.c	(working copy)
@@ -112,8 +112,8 @@  reload_cse_simplify (rtx insn, rtx testr
 	  if (REG_P (value)
 	      && ! REG_FUNCTION_VALUE_P (value))
 	    value = 0;
-	  check_for_inc_dec (insn);
-	  delete_insn_and_edges (insn);
+	  if (check_for_inc_dec (insn, NULL))
+	    delete_insn_and_edges (insn);
 	  return;
 	}
 
@@ -164,8 +164,8 @@  reload_cse_simplify (rtx insn, rtx testr
 
       if (i < 0)
 	{
-	  check_for_inc_dec (insn);
-	  delete_insn_and_edges (insn);
+	  if (check_for_inc_dec (insn, NULL))
+	    delete_insn_and_edges (insn);
 	  /* We're done with this insn.  */
 	  return;
 	}
Index: regset.h
===================================================================
--- regset.h	(revision 180683)
+++ regset.h	(working copy)
@@ -1,6 +1,6 @@ 
 /* Define regsets.
    Copyright (C) 1987, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004,
-   2005, 2006, 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+   2005, 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -115,6 +115,9 @@  #define EXECUTE_IF_AND_IN_REG_SET(REGSET
 
 extern regset regs_invalidated_by_call_regset;
 
+/* Same information as FIXED_REG_SET but in regset form.  */
+extern regset fixed_regset;
+
 /* An obstack for regsets.  */
 extern bitmap_obstack reg_obstack;
 
Index: dse.c
===================================================================
--- dse.c	(revision 180683)
+++ dse.c	(working copy)
@@ -33,6 +33,7 @@  Software Foundation; either version 3, o
 #include "tm_p.h"
 #include "regs.h"
 #include "hard-reg-set.h"
+#include "regset.h"
 #include "flags.h"
 #include "df.h"
 #include "cselib.h"
@@ -377,6 +378,13 @@  struct insn_info
      created.  */
   read_info_t read_rec;
 
+  /* The live fixed registers.  We assume only fixed registers can
+     cause trouble by being clobbered from an expanded pattern;
+     storing only the live fixed registers (rather than all registers)
+     means less memory needs to be allocated / copied for the individual
+     stores.  */
+  regset fixed_regs_live;
+
   /* The prev insn in the basic block.  */
   struct insn_info * prev_insn;
 
@@ -448,9 +456,9 @@  struct bb_info
   /* The following bitvector is indexed by the reg number.  It
      contains the set of regs that are live at the current instruction
      being processed.  While it contains info for all of the
-     registers, only the pseudos are actually examined.  It is used to
-     assure that shift sequences that are inserted do not accidently
-     clobber live hard regs.  */
+     registers, only the hard registers are actually examined.  It is used
+     to assure that shift and/or add sequences that are inserted do not
+     accidently clobber live hard regs.  */
   bitmap regs_live;
 };
 
@@ -827,6 +835,51 @@  free_store_info (insn_info_t insn_info)
   insn_info->store_rec = NULL;
 }
 
+typedef struct
+{
+  rtx insert_before;
+  rtx first, current;
+  regset fixed_regs_live;
+  bool failure;
+} note_add_store_info;
+
+/* Callback for emit_inc_dec_insn_before via note_stores.
+   Check if a register is clobbered which is life afterwards.  */
+static void
+note_add_store (rtx loc, const_rtx expr ATTRIBUTE_UNUSED, void *data)
+{
+  rtx insn, *nextp;
+  note_add_store_info *info = (note_add_store_info *) data;
+  int r, n;
+
+  if (!REG_P (loc))
+    return;
+  /* If this register is referenced by the current or an earlier insn,
+     that's OK.  E.g. this applies to the register that is being incremented
+     with this addition.  */
+  nextp = &info->first;
+  do
+    {
+      insn = *nextp;
+      nextp = &NEXT_INSN (insn);
+      if (reg_referenced_p (loc, PATTERN (insn)))
+	return;
+    }
+  while (insn != info->current);
+
+  if (!info->fixed_regs_live)
+    {
+      info->failure =  true;
+      return;
+    }
+  /* Now check if this is a live fixed register.  */
+  r = REGNO (loc);
+  n = HARD_REGNO_NREGS (r, GET_MODE (loc));
+  while (--n >=  0)
+    if (REGNO_REG_SET_P (info->fixed_regs_live, r+n))
+      info->failure =  true;
+}
+
 /* Callback for for_each_inc_dec that emits an INSN that sets DEST to
    SRC + SRCOFF before insn ARG.  */
 
@@ -835,31 +888,62 @@  emit_inc_dec_insn_before (rtx mem ATTRIB
 			  rtx op ATTRIBUTE_UNUSED,
 			  rtx dest, rtx src, rtx srcoff, void *arg)
 {
-  rtx insn = (rtx)arg;
-
-  if (srcoff)
-    src = gen_rtx_PLUS (GET_MODE (src), src, srcoff);
+  insn_info_t insn_info = (insn_info_t) arg;
+  rtx insn = insn_info->insn, new_insn, cur;
+  note_add_store_info info;
 
   /* We can reuse all operands without copying, because we are about
      to delete the insn that contained it.  */
-
-  emit_insn_before (gen_rtx_SET (VOIDmode, dest, src), insn);
+  if (srcoff)
+    new_insn = gen_add3_insn (dest, src, srcoff);
+  else
+    new_insn = gen_move_insn (dest, src);
+  info.insert_before = insn;
+  info.first = new_insn;
+  info.fixed_regs_live = insn_info->fixed_regs_live;
+  info.failure = false;
+  for (cur = new_insn; cur; cur = NEXT_INSN (cur))
+    {
+      info.current = cur;
+      note_stores (PATTERN (cur), note_add_store, &info);
+    }
+  if (info.failure)
+    return 1;
+  emit_insn_before (new_insn, insn);
 
   return -1;
 }
 
 /* Before we delete INSN, make sure that the auto inc/dec, if it is
-   there, is split into a separate insn.  */
+   there, is split into a separate insn.
+   Return true on success (or if there was nothing to do), false on failure.  */
 
-void
-check_for_inc_dec (rtx insn)
+static bool
+check_for_inc_dec_1 (insn_info_t insn_info)
 {
+  rtx insn = insn_info->insn;
   rtx note = find_reg_note (insn, REG_INC, NULL_RTX);
   if (note)
-    for_each_inc_dec (&insn, emit_inc_dec_insn_before, insn);
+    return for_each_inc_dec (&insn, emit_inc_dec_insn_before, insn_info) == 0;
+  return true;
 }
 
 
+/* Entry point for postreload.  */
+bool
+check_for_inc_dec (rtx insn, regset fixed_regs_live)
+{
+  struct insn_info insn_info;
+  rtx note;
+
+  insn_info.insn = insn;
+  insn_info.fixed_regs_live = fixed_regs_live;
+  note = find_reg_note (insn, REG_INC, NULL_RTX);
+  if (note)
+    return for_each_inc_dec (&insn, emit_inc_dec_insn_before, &insn_info) == 0;
+  return true;
+}
+
 /* Delete the insn and free all of the fields inside INSN_INFO.  */
 
 static void
@@ -870,7 +954,8 @@  delete_dead_store_insn (insn_info_t insn
   if (!dbg_cnt (dse))
     return;
 
-  check_for_inc_dec (insn_info->insn);
+  if (!check_for_inc_dec_1 (insn_info))
+    return;
   if (dump_file)
     {
       fprintf (dump_file, "Locally deleting insn %d ",
@@ -2375,6 +2460,16 @@  get_call_args (rtx call_insn, tree fn, r
   return true;
 }
 
+/* Return a bitmap of the fixed registers contained in IN.  */
+static bitmap
+copy_fixed_regs (const_bitmap in)
+{
+  bitmap ret;
+
+  ret = ALLOC_REG_SET (NULL);
+  bitmap_and (ret, in, fixed_regset);
+  return ret;
+}
 
 /* Apply record_store to all candidate stores in INSN.  Mark INSN
    if some part of it is not a candidate store and assigns to a
@@ -2529,6 +2624,8 @@  scan_insn (bb_info_t bb_info, rtx insn)
 			  active_local_stores_len = 1;
 			  active_local_stores = NULL;
 			}
+		      insn_info->fixed_regs_live
+			= copy_fixed_regs (bb_info->regs_live);
 		      insn_info->next_local_store = active_local_stores;
 		      active_local_stores = insn_info;
 		    }
@@ -2579,6 +2676,7 @@  scan_insn (bb_info_t bb_info, rtx insn)
 	  active_local_stores_len = 1;
 	  active_local_stores = NULL;
 	}
+      insn_info->fixed_regs_live = copy_fixed_regs (bb_info->regs_live);
       insn_info->next_local_store = active_local_stores;
       active_local_stores = insn_info;
     }
@@ -3622,9 +3720,9 @@  dse_step5_nospill (void)
 		}
 	      if (deleted)
 		{
-		  if (dbg_cnt (dse))
+		  if (dbg_cnt (dse)
+		      && check_for_inc_dec_1 (insn_info))
 		    {
-		      check_for_inc_dec (insn_info->insn);
 		      delete_insn (insn_info->insn);
 		      insn_info->insn = NULL;
 		      globally_deleted++;
@@ -3702,12 +3800,12 @@  dse_step5_spill (void)
 		    deleted = false;
 		  store_info = store_info->next;
 		}
-	      if (deleted && dbg_cnt (dse))
+	      if (deleted && dbg_cnt (dse)
+		  && check_for_inc_dec_1 (insn_info))
 		{
 		  if (dump_file)
 		    fprintf (dump_file, "Spill deleting insn %d\n",
 			     INSN_UID (insn_info->insn));
-		  check_for_inc_dec (insn_info->insn);
 		  delete_insn (insn_info->insn);
 		  spill_deleted++;
 		  insn_info->insn = NULL;
Index: rtl.h
===================================================================
--- rtl.h	(revision 180683)
+++ rtl.h	(working copy)
@@ -2396,7 +2396,7 @@  extern int exp_equiv_p (const_rtx, const
 extern unsigned hash_rtx (const_rtx x, enum machine_mode, int *, int *, bool);
 
 /* In dse.c */
-extern void check_for_inc_dec (rtx insn);
+extern bool check_for_inc_dec (rtx insn, bitmap);
 
 /* In jump.c */
 extern int comparison_dominates_p (enum rtx_code, enum rtx_code);
Index: reginfo.c
===================================================================
--- reginfo.c	(revision 180683)
+++ reginfo.c	(working copy)
@@ -1,7 +1,7 @@ 
 /* Compute different info about registers.
    Copyright (C) 1987, 1988, 1991, 1992, 1993, 1994, 1995, 1996
    1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008,
-   2009, 2010  Free Software Foundation, Inc.
+   2009, 2010, 2011  Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -94,6 +94,9 @@  static tree GTY(()) global_regs_decl[FIR
    in dataflow more conveniently.  */
 regset regs_invalidated_by_call_regset;
 
+/* Same information as fixed_reg_set but in regset form.  */
+regset fixed_regset;
+
 /* The bitmap_obstack is used to hold some static variables that
    should not be reset after each function is compiled.  */
 static bitmap_obstack persistent_obstack;
@@ -451,6 +454,10 @@  init_reg_sets_1 (void)
     }
   else
     CLEAR_REG_SET (regs_invalidated_by_call_regset);
+  if (!fixed_regset)
+    fixed_regset = ALLOC_REG_SET (&persistent_obstack);
+  else
+    CLEAR_REG_SET (fixed_regset);
 
   for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
     {
@@ -462,7 +469,10 @@  init_reg_sets_1 (void)
 #endif
 
       if (fixed_regs[i])
-	SET_HARD_REG_BIT (fixed_reg_set, i);
+	{
+	  SET_HARD_REG_BIT (fixed_reg_set, i);
+	  SET_REGNO_REG_SET (fixed_regset, i);
+	}
 
       if (call_used_regs[i])
 	SET_HARD_REG_BIT (call_used_reg_set, i);