This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
reload cse may delete loads it shouldn't
- From: Alexandre Oliva <aoliva at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org
- Date: 15 Apr 2003 05:55:30 -0300
- Subject: reload cse may delete loads it shouldn't
- Organization: GCC Team, Red Hat
A function as simple as this will miscompile on mn10300-elf -mam33 -O2
-fno-force-mem. A far more complex function might fail even without
-fno-force-mem, but it's tricky to force it into the right conditions,
so I can't tell for sure.
int k;
int f(int p, unsigned char *q) {
int j = *q + 512;
*q = k = j;
return p / *q;
}
After greg, we have something like this:
(set (reg:SI d1) (plus:SI (reg:SI d1) (const_int 512)))
;; ...
(set (mem:QI (reg:SI a0)) (reg:QI d1)) ;; *q = /* k = */ j;
(set (reg:QI d1) (mem:QI (reg:SI a0))) ;; reload *q for...
(parallel [(set (reg:SI d0) (div:SI (reg:SI d0) (reg:SI d1)))
(set (reg:SI d1) (mod:SI (reg:SI d0) (reg:SI d1)))])
The problem is that we assume that, after the store of d1 in *q,
loading it back won't modify the value in d1, so reload cse deletes
the (zero-extending) load, and we end up using the SImode register
that contains *q + 512, instead of just *q.
Ideally, we should be replacing the reload instruction with a simpler
zero-extension, but I still haven't figured out why we don't do this
on mn10300, even though we seem to do it on x86. I suspect it has to
do with the absence of LOAD_EXTEND_OP on i386.h, but I still haven't
verified this assumption.
Anyway, this patch is meant to fix the correctness issue in reload
cse. The idea of the patch is to record the mode in which a register
was set, and only drop a copy from the same equivalence class if it
has the same mode as the original set. Copying any different mode may
alter the contents of the register, so we don't drop such a copy.
I'm still going to bootstrap this to check for correctness, but does
the idea sound sound? :-)
Ok to install if it bootstrap on athlon-pc-linux-gnu?
Index: gcc/ChangeLog
from Alexandre Oliva <aoliva at redhat dot com>
* reload1.c (reload_cse_noop_set_p): Return false if mode of
SET_DEST is not the same as that returned by...
* cselib.h (cselib_reg_set_mode): ... new function.
* cselib.c (cselib_reg_set_mode): Define it.
(REG_VALUES): Document semantics of first element as set mode.
(cselib_subst_to_values): Skip first element if ELT is NULL.
(cselib_lookup): Likewise. Insert past the first element.
(cselib_invalidate_regno): NULLify first element.
(cselib_record_set): Set first element.
Index: gcc/reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.387
diff -u -p -r1.387 reload1.c
--- gcc/reload1.c 9 Apr 2003 17:50:08 -0000 1.387
+++ gcc/reload1.c 15 Apr 2003 08:29:38 -0000
@@ -8025,6 +8025,9 @@ static int
reload_cse_noop_set_p (set)
rtx set;
{
+ if (cselib_reg_set_mode (SET_DEST (set)) != GET_MODE (SET_DEST (set)))
+ return 0;
+
return rtx_equal_for_cselib_p (SET_DEST (set), SET_SRC (set));
}
Index: gcc/cselib.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cselib.h,v
retrieving revision 1.7
diff -u -p -r1.7 cselib.h
--- gcc/cselib.h 11 Mar 2003 21:52:41 -0000 1.7
+++ gcc/cselib.h 15 Apr 2003 08:29:38 -0000
@@ -1,6 +1,6 @@
/* Common subexpression elimination for GNU compiler.
Copyright (C) 1987, 1988, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
- 1999 Free Software Foundation, Inc.
+ 1999, 2003 Free Software Foundation, Inc.
This file is part of GCC.
@@ -67,6 +67,7 @@ extern void cselib_update_varray_sizes P
extern void cselib_init PARAMS ((void));
extern void cselib_finish PARAMS ((void));
extern void cselib_process_insn PARAMS ((rtx));
+extern enum machine_mode cselib_reg_set_mode PARAMS ((rtx));
extern int rtx_equal_for_cselib_p PARAMS ((rtx, rtx));
extern int references_value_p PARAMS ((rtx, int));
extern rtx cselib_subst_to_values PARAMS ((rtx));
Index: gcc/cselib.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cselib.c,v
retrieving revision 1.26
diff -u -p -r1.26 cselib.c
--- gcc/cselib.c 14 Mar 2003 20:15:11 -0000 1.26
+++ gcc/cselib.c 15 Apr 2003 08:29:38 -0000
@@ -1,6 +1,6 @@
/* Common subexpression elimination library for GNU compiler.
Copyright (C) 1987, 1988, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
- 1999, 2000, 2001 Free Software Foundation, Inc.
+ 1999, 2000, 2001, 2003 Free Software Foundation, Inc.
This file is part of GCC.
@@ -99,9 +99,13 @@ static int n_useless_values;
/* Number of useless values before we remove them from the hash table. */
#define MAX_USELESS_VALUES 32
-/* This table maps from register number to values. It does not contain
- pointers to cselib_val structures, but rather elt_lists. The purpose is
- to be able to refer to the same register in different modes. */
+/* This table maps from register number to values. It does not
+ contain pointers to cselib_val structures, but rather elt_lists.
+ The purpose is to be able to refer to the same register in
+ different modes. The first element of the list defines the mode in
+ which the register was set; if the mode is unknown or the value is
+ no longer valid in that mode, ELT will be NULL for the first
+ element. */
static GTY(()) varray_type reg_values;
static GTY((deletable (""))) varray_type reg_values_old;
#define REG_VALUES(I) VARRAY_ELT_LIST (reg_values, (I))
@@ -402,6 +406,25 @@ remove_useless_values ()
abort ();
}
+/* Return the mode in which a register was last set. If X is not a
+ register, return its mode. If the mode in which the register was
+ set is not known, or the value was already clobbered, return
+ VOIDmode. */
+
+enum machine_mode
+cselib_reg_set_mode (x)
+ rtx x;
+{
+ if (GET_CODE (x) != REG)
+ return GET_MODE (x);
+
+ if (REG_VALUES (REGNO (x)) == NULL
+ || REG_VALUES (REGNO (x))->elt == NULL)
+ return VOIDmode;
+
+ return GET_MODE (REG_VALUES (REGNO (x))->elt->u.val_rtx);
+}
+
/* Return nonzero if we can prove that X and Y contain the same value, taking
our gathered information into account. */
@@ -812,7 +835,10 @@ cselib_subst_to_values (x)
switch (code)
{
case REG:
- for (l = REG_VALUES (REGNO (x)); l; l = l->next)
+ l = REG_VALUES (REGNO (x));
+ if (l && l->elt == NULL)
+ l = l->next;
+ for (; l; l = l->next)
if (GET_MODE (l->elt->u.val_rtx) == GET_MODE (x))
return l->elt->u.val_rtx;
@@ -909,7 +935,10 @@ cselib_lookup (x, mode, create)
struct elt_list *l;
unsigned int i = REGNO (x);
- for (l = REG_VALUES (i); l; l = l->next)
+ l = REG_VALUES (i);
+ if (l && l->elt == NULL)
+ l = l->next;
+ for (; l; l = l->next)
if (mode == GET_MODE (l->elt->u.val_rtx))
return l->elt;
@@ -927,8 +956,11 @@ cselib_lookup (x, mode, create)
e = new_cselib_val (++next_unknown_value, GET_MODE (x));
e->locs = new_elt_loc_list (e->locs, x);
if (REG_VALUES (i) == 0)
- VARRAY_PUSH_UINT (used_regs, i);
- REG_VALUES (i) = new_elt_list (REG_VALUES (i), e);
+ {
+ VARRAY_PUSH_UINT (used_regs, i);
+ REG_VALUES (i) = new_elt_list (REG_VALUES (i), NULL);
+ }
+ REG_VALUES (i)->next = new_elt_list (REG_VALUES (i)->next, e);
slot = htab_find_slot_with_hash (hash_table, x, e->value, INSERT);
*slot = e;
return e;
@@ -1011,17 +1043,23 @@ cselib_invalidate_regno (regno, mode)
struct elt_loc_list **p;
unsigned int this_last = i;
- if (i < FIRST_PSEUDO_REGISTER)
+ if (i < FIRST_PSEUDO_REGISTER && v != NULL)
this_last += HARD_REGNO_NREGS (i, GET_MODE (v->u.val_rtx)) - 1;
- if (this_last < regno)
+ if (this_last < regno || v == NULL)
{
l = &(*l)->next;
continue;
}
/* We have an overlap. */
- unchain_one_elt_list (l);
+ if (*l == REG_VALUES (i))
+ {
+ (*l)->elt = NULL;
+ l = &(*l)->next;
+ }
+ else
+ unchain_one_elt_list (l);
/* Now, we clear the mapping from value to reg. It must exist, so
this code will crash intentionally if it doesn't. */
@@ -1224,7 +1262,15 @@ cselib_record_set (dest, src_elt, dest_a
max_value_regs = n;
}
- REG_VALUES (dreg) = new_elt_list (REG_VALUES (dreg), src_elt);
+ if (REG_VALUES (dreg))
+ {
+ if (REG_VALUES (dreg)->elt)
+ abort ();
+ else
+ REG_VALUES (dreg)->elt = src_elt;
+ }
+ else
+ REG_VALUES (dreg) = new_elt_list (REG_VALUES (dreg), src_elt);
if (src_elt->locs == 0)
n_useless_values--;
src_elt->locs = new_elt_loc_list (src_elt->locs, dest);
--
Alexandre Oliva Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer aoliva at {redhat dot com, gcc.gnu.org}
CS PhD student at IC-Unicamp oliva at {lsd dot ic dot unicamp dot br, gnu.org}
Free Software Evangelist Professional serial bug killer