This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[RFA:] Use of find_reg_note in FIND_REG_INC_NOTE causes dbr failure
- To: gcc-patches at gcc dot gnu dot org
- Subject: [RFA:] Use of find_reg_note in FIND_REG_INC_NOTE causes dbr failure
- From: Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>
- Date: Thu, 13 Sep 2001 05:16:25 +0200
First, speaking of dbr-related failures, here's an unreviewed patch:
<URL:http://gcc.gnu.org/ml/gcc-patches/2001-08/msg00575.html> (Patch for
bug in reorg.c: fill_slots_from_thread missing update of own_thread).
Thanks in advance for reviewing that and this one.
Now to the one at hand: for CRIS, gcc.c-torture/execute/memcpy-bi.c,
string-opt-12.c and string-opt-5.c fail without this patch because there's
wrong code after the dbr pass (at least for memcpy-bi.c): the wrong
register is substituted for one of the moves from src. But for once, it
doesn't seem to be a bug in reorg.c.
The function reg_set_p uses REG_INC_NOTE which checks pointer equality of
the note. The call comes from reorg.c:fill_slots_from_thread, where
incidentally pointer equality for registers is not guaranteed;
regrename.c:do_replace can create duplicate REG rtx:es. That's not the
only place where pseudos can be mapped to the same hard reg, but here we
have the *same* pseudo in different rtx:es, so worth mentioning. The code
in reorg.c:fill_slots_from_thread where the bad substitution happens is:
if (GET_CODE (trial) == INSN && GET_CODE (pat) == SET
&& GET_CODE (SET_SRC (pat)) == REG
&& GET_CODE (SET_DEST (pat)) == REG)
{
rtx next = next_nonnote_insn (trial);
if (next && GET_CODE (next) == INSN
&& GET_CODE (PATTERN (next)) != USE
&& ! reg_set_p (SET_DEST (pat), next)
&& ! reg_set_p (SET_SRC (pat), next)
&& reg_referenced_p (SET_DEST (pat), PATTERN (next))
&& ! modified_in_p (SET_DEST (pat), next))
validate_replace_rtx (SET_DEST (pat), SET_SRC (pat), next);
}
The insn "trial" (container of "pat", the BODY of "trial") is:
(insn 433 146 148 (set (reg:SI 2 r2 [57])
(reg/f:SI 4 r4 [105])) 32 {*movsi_internal} (insn_list 146 (insn_list 146 (nil)))
(nil))
"next" is:
(insn 477 148 149 (set (reg:SI 13 r13)
(mem/s:SI (post_inc:SI (reg:SI 2 r2 [57])) 0)) 32 {*movsi_internal} (insn_list 433 (nil))
(expr_list:REG_INC (reg:SI 2 r2 [57])
(nil)))
The (reg:SI 2 r2 [57]) in the REG_INC note in "next" and the one in
"trial" are different objects (pointerwise), as allocated by
regrename.c:do_replace. The first "&& ! reg_set_p (SET_DEST (pat), next)"
should have caught this, but reg_set_p uses FIND_REG_INC_NOTE (insn, reg)
to guard against the presence of postincrements such as this one. But
FIND_REG_INC_NOTE is for autoinc machines defined to "(find_reg_note
((insn), REG_INC, (reg)))", and the function find_reg_note compares
pointers when checking "reg" for a match. So it fails to match the
duplicated REG it is passed. Any required change seems to be limited to
the definition of FIND_REG_INC_NOTE, its callers and the function
find_reg_note. I've taken one of the possible routes.
There's a neighboring function, find_regno_note, that seems better for the
case at hand, since it tests register-number equality, and also overlap
(for hard registers). The drawback is that it wants the REG_INC note to
actually be for a *register*, and does not find other REG_INCs.
Many current callers of FIND_REG_INC_NOTE pass NULL_RTX for the note to
match, and could just as well be using side_effects_p as a change "Fri Aug
6 16:53:55 EDT 1999 John Wehle (john@feith.com)" in ChangeLog.2 indicates,
if it weren't for that for non-autoinc machines FIND_REG_INC_NOTE is
defined to 0. Most of the rest of the callers test elsewhere that the
REG_INCs they pass, or the non-zero return-value, is a REG, and so would
be better off with FIND_REG_INC_NOTE using find_regno_note. The exception
is a use in reload.c, but it seems to be a case not normally encountered
(an autoincrement of a MEM that needs to be replaced) so it could just as
well use find_reg_note directly. But changing FIND_REG_INC_NOTE to call
find_regno_note for REGs seems best. The (rtx) casts are necessary
because some callers pass 0, not NULL_RTX.
Built and checked with no regressions on h8300-hms+h8300-sim (with
--enable-languages=objc), mn10300-elf+mn10300-sim and
powerpc-eabisim+powerpc-sim. The other autoinc simulator targets don't
build with sources as of "Wed Sep 12 09:04:36 GMT 2001". Note that
i686-pc-linux-gnu is not an autoinc machine and isn't affected by this
change. In order to get the patch tested on more autoinc machines, I
applied kaz Kojima's patch at
<URL:http://gcc.gnu.org/ml/gcc-patches/2001-09/msg00009.html> and built
and tested on sh-elf+sh-hms-sim too (with that patch applied only for that
target), with no regressions.
Ok to commit?
* rtl.h (FIND_REG_INC_NOTE) [HAVE_PRE_INCREMENT
|| HAVE_PRE_DECREMENT || HAVE_POST_INCREMENT
|| HAVE_POST_DECREMENT]: Call find_regno_note for REGs.
Index: rtl.h
===================================================================
RCS file: /cvs/gcc/egcs/gcc/rtl.h,v
retrieving revision 1.294
diff -p -c -r1.294 rtl.h
*** rtl.h 2001/08/30 20:44:50 1.294
--- rtl.h 2001/09/12 03:47:22
*************** extern unsigned int subreg_regno PARAMS
*** 944,950 ****
/* Don't continue this line--convex cc version 4.1 would lose. */
#if (defined (HAVE_PRE_INCREMENT) || defined (HAVE_PRE_DECREMENT) || defined (HAVE_POST_INCREMENT) || defined (HAVE_POST_DECREMENT))
! #define FIND_REG_INC_NOTE(insn, reg) (find_reg_note ((insn), REG_INC, (reg)))
#else
#define FIND_REG_INC_NOTE(insn, reg) 0
#endif
--- 944,953 ----
/* Don't continue this line--convex cc version 4.1 would lose. */
#if (defined (HAVE_PRE_INCREMENT) || defined (HAVE_PRE_DECREMENT) || defined (HAVE_POST_INCREMENT) || defined (HAVE_POST_DECREMENT))
! #define FIND_REG_INC_NOTE(insn, reg) \
! (reg != NULL_RTX && REG_P ((rtx) (reg)) \
! ? find_regno_note ((insn), REG_INC, REGNO ((rtx) (reg))) \
! : find_reg_note ((insn), REG_INC, (reg)))
#else
#define FIND_REG_INC_NOTE(insn, reg) 0
#endif
brgds, H-P