Re: RFA: Fix dse / postreload not to bypass add expanders
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
Re: RFA: Fix dse / postreload not to bypass add expanders
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. -- Eric Botcazou
Re: RFA: Fix dse / postreload not to bypass add expanders
Quoting Paolo Bonzini bonz...@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.
Re: RFA: Fix dse / postreload not to bypass add expanders
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.renne...@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
RFA: Fix dse / postreload not to bypass add expanders
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.renne...@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.
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
