This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
reorg bug + patch
- To: gcc-patches at gcc dot gnu dot org
- Subject: reorg bug + patch
- From: Toshiyasu Morita <tm at netcom dot com>
- Date: Thu, 6 Jan 2000 02:56:14 -0800 (PST)
- Cc: amylaar at cygnus dot com, taruna at cygnus dot com
I believe I have found a fairly nasty bug in reorg that's been plaguing
us for a while, with a partial patch for the problem. I originally reported
this problem as Cygnus case 102925.
The problem occurs on targets with a delayed branch instruction
and an addressing mode with side effects (e.g. postinc, etc), such
as the Hitachi SH.
Here is a dump of the rtx just prior to the delayed branch filling pass:
(insn 834 832 4541 (set (reg:SI 1 r1)
(mem/u:SI (label_ref 6079))) 123 {movsi_ie} (nil)
(expr_list:REG_EQUIV (symbol_ref:SI ("ball_obj"))
(nil)))
(insn 4541 834 4919 (set (reg:SI 2 r2)
(reg:SI 1 r1)) 123 {movsi_ie} (insn_list 834 (nil))
(nil))
(insn 4919 4541 4916 (parallel[
(set (reg:SF 24 fr0)
(mem/s:SF (post_inc:SI (reg:SI 2 r2)))) <- look here
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (nil)
(nil))
(insn 4916 4919 836 (set (reg:SI 0 r0)
(const_int 52)) 123 {movsi_ie} (nil)
(nil))
(insn:HI 836 4916 837 (parallel[
(set (mem:SF (plus:SI (reg:SI 14 r14)
(reg:SI 0 r0)))
(reg:SF 24 fr0))
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (insn_list 4541 (nil))
(expr_list:REG_DEAD (reg:SF 24 fr0)
(expr_list:REG_INC (reg:SI 2 r2)
(expr_list:REG_UNUSED (scratch:SI)
(nil)))))
(note 837 836 842 ("player.i") 23020)
(note 842 837 4925 "" NOTE_INSN_DELETED)
(insn 4925 842 4922 (parallel[
(set (reg:SF 24 fr0)
(mem/s:SF (reg:SI 2 r2)))
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (nil)
(nil))
Here are the same insns after delayed branch filling:
; Insn is in multiple basic blocks
(insn 834 832 4541 (set (reg:SI 1 r1)
(mem/u:SI (label_ref 6079))) 123 {movsi_ie} (nil)
(expr_list:REG_EQUIV (symbol_ref:SI ("ball_obj"))
(nil)))
;; Insn is in multiple basic blocks
(insn 4541 834 4919 (set (reg:SI 2 r2)
(reg:SI 1 r1)) 123 {movsi_ie} (insn_list 834 (nil))
(nil))
;; Insn is in multiple basic blocks
(insn 4919 4541 836 (parallel[
(set (reg:SF 24 fr0)
(mem/s:SF (post_inc:SI (reg:SI 1 r1)))) <- look here
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (nil)
(nil))
;; Insn is in multiple basic blocks
(insn:HI 836 4919 837 (parallel[
(set (mem:SF (plus:SI (reg:SI 14 r14)
(reg:SI 0 r0)))
(reg:SF 24 fr0))
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (insn_list 4541 (nil))
(expr_list:REG_DEAD (reg:SF 24 fr0)
(expr_list:REG_INC (reg:SI 2 r2)
(expr_list:REG_UNUSED (scratch:SI)
(nil)))))
;; Insn is in multiple basic blocks
(note 837 836 842 ("player.i") 23020)
;; Insn is in multiple basic blocks
(note 842 837 4925 "" NOTE_INSN_DELETED)
;; Insn is in multiple basic blocks
(insn 4925 842 4922 (parallel[
(set (reg:SF 24 fr0)
(mem/s:SF (reg:SI 2 r2)))
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (nil)
(nil))
The culprit appears to be this chunk of code in reorg.c:
/* If this insn is a register-register copy and the next insn has
a use of our destination, change it to use our source. That way,
it will become a candidate for our delay slot the next time
through this loop. This case occurs commonly in loops that
scan a list.
We could check for more complex cases than those tested below,
but it doesn't seem worth it. It might also be a good idea to try
to swap the two insns. That might do better.
We can't do this if the next insn modifies our destination, because
that would make the replacement into the insn invalid. We also can't
do this if it modifies our source, because it might be an earlyclobber
operand. This latter test also prevents updating the contents of
a PRE_INC. */
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 *** breakpoint here ***
&& 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)))
validate_replace_rtx (SET_DEST (pat), SET_SRC (pat), next);
}
}
(xxgdb) call debug_rtx(trial)
(insn 4541 834 4919 (set (reg:SI 2 r2)
(reg:SI 1 r1)) 123 {movsi_ie} (insn_list 834 (nil))
(nil))
(xxgdb) call debug_rtx(pat)
(set (reg:SI 2 r2)
(reg:SI 1 r1)
(xxgdb) call debug_rtx(next)
(insn 4919 4541 836 (parallel[
(set (reg:SF 24 fr0)
(mem/s:SF (post_inc:SI (reg:SI 1 r1))))
(use (reg/v:PSI 48 fpscr))
(clobber (scratch:SI))
] ) 157 {movsf_ie} (nil)
(nil))
At this point, validate_replace_rtx() is called with:
arg0: (reg:SI 2 r2)
arg1: (reg:SI 1 r1)
arg2: insn 4919
...and the incorrect code is generated.
Even though the comment for the code mentions checking the destination
for a modification, the code does not seem to check this!
The correct solution seem to be to add a !modified_in_p (SET_DEST (pat))
to the second conditional, but this doesn't work currently because
global register alloc added the REG_INC (reg: SI 2 r2) note to the
wrong insn! (insn 836)
Anyway, here is my first patch to fix the first problem:
Jan 6 02:14:33 PST 2000 Toshiyasu Morita (toshi.morita@sega.com)
* reorg.c (fill_slots_from_thread): Actually check modified_in_p()
before replacing rtx.
*** reorg.c.bak Thu Jan 6 02:25:19 2000
--- reorg.c Thu Jan 6 02:24:15 2000
*************** fill_slots_from_thread (insn, condition,
*** 3718,3723 ****
--- 3718,3726 ----
operand. This latter test also prevents updating the contents of
a PRE_INC. */
+ /* This code should probably be disabled when regmove is enabled
+ because it interferes with proper address inheritance. */
+
if (GET_CODE (trial) == INSN && GET_CODE (pat) == SET
&& GET_CODE (SET_SRC (pat)) == REG
&& GET_CODE (SET_DEST (pat)) == REG)
*************** fill_slots_from_thread (insn, condition,
*** 3729,3734 ****
--- 3732,3738 ----
&& ! 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);
}
}
Toshi