This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[patch] m32r: Fix so many (24) testsuite failures. (take 2)
- From: Kazu Hirata <kazu at cs dot umass dot edu>
- To: gcc-patches at gcc dot gnu dot org
- Cc: nickc at redhat dot com, inaoka dot kazuhiro at renesas dot com
- Date: Mon, 22 Dec 2003 11:54:14 -0500 (EST)
- Subject: [patch] m32r: Fix so many (24) testsuite failures. (take 2)
Hi,
Attached is a patch to fix the block move pattern in m32r.md.
Analyzing the failure of execute/20000412-3.c, I found that wrong
registers are assigned to movstrsi_internal pattern. Specifically,
one register, say (reg:SI 5), appears in two places like so.
(parallel [(...)
(set (reg:SI 5) (...))
(clobber (reg:SI 5))
(...)])
This does not look good. This error is picked up by the regrename
pass. After it does all sorts of weired renumbering, the test case
fails.
The problem lies in the definition of movstrsi_internal. Unlike other
block move patterns found in GCC, the pattern does not simply clobber
the source pointer but leave it with the address of the memory
location just after the copied block. The destination pointer is
updated likewise. The problem is that these pointers are updated with
(set (match_dup 0) (...))
(set (match_dup 1) (...))
(Yes, match_dup is in the set destination!) These should really be
(set (match_operand:SI 3 "register_operand" "=0") (...))
(set (match_operand:SI 4 "register_operand" "=1") (...))
to have the register allocator match operands[0] and operands[3], not
on our own with (match_dup x). But notice that these operands[3] and
operands[4] are new registers and thus would die at the insn if nobody
used them. To prevent them from dying, we need to copy these back to
the original source and destination pointers. (Look for
emit_move_insn() in the patch.)
As to the (set (mem:BLK) (mem:BLK)) part of the pattern, I think the
constraints should be OK with 'r'. The modifications to these two
registers are described in the remaining part of the pattern.
While I was working on the fix, I found a latent bug in
m32r_output_block_move(). The pattern says the source and the
destination pointers are incremented by some amount, but
m32r_output_block_move() does not completely follow that. Consider a
block move of 3 bytes for example.
ld r7, @r1 ; load the first 4 bytes into a temporary
sra3 r6, r7, #16 ; get the first 2 bytes into r6 by shifting
sth r6, @(0,r0) ; store the first two bytes
srai r7, #8 ; get the 3rd byte by shifting
stb r7, @(2,r0) ; store the 3rd byte
where r1 and r0 are the source and destination pointer, respectively.
Note that r0 or r1 are not modified despite what the block move
pattern says. I circumvent this problem by outputting adjustments so
that the pattern matches the actual generated code. (Look for
"Update" in the patch.) You might argue that it doesn't make sense to
update these pointers just for the sake of doing what the pattern
says, but I've already crammed so much into one patch. I will
probably post a follow-up patch to get rid of these pointer updates.
Other changes in m32r.c are mostly renumbering of operand numbers,
namely 3->5 and 4->6.
To fix the main problem, I got ideas from PR 12612, 12630, and 12978.
Tested on m32r-elf. The following failures gone. With the patch, the
test result is as clean as one with the block move instruction
completely removed.
-FAIL: gcc.c-torture/compile/20010328-1.c (test for excess errors)
-FAIL: gcc.c-torture/execute/20000412-3.c execution
-FAIL: gcc.c-torture/execute/20011113-1.c execution
-FAIL: gcc.c-torture/execute/20011121-1.c execution
-FAIL: gcc.c-torture/execute/20020206-1.c execution
-FAIL: gcc.c-torture/execute/20020215-1.c execution
-FAIL: gcc.c-torture/execute/20030914-1.c execution
-FAIL: gcc.c-torture/execute/20030914-2.c execution
-FAIL: gcc.c-torture/execute/920625-1.c execution
-FAIL: gcc.c-torture/execute/921013-1.c execution
-FAIL: gcc.c-torture/execute/921117-1.c execution
-FAIL: gcc.c-torture/execute/931004-11.c execution
-FAIL: gcc.c-torture/execute/990628-1.c compilation
-FAIL: gcc.c-torture/execute/builtins/string-5.c execution
-FAIL: gcc.c-torture/execute/builtins/string-7.c execution
-FAIL: gcc.c-torture/execute/memcpy-1.c execution
-FAIL: gcc.c-torture/execute/memcpy-bi.c execution
-FAIL: gcc.c-torture/execute/string-opt-10.c execution
-FAIL: gcc.c-torture/execute/string-opt-12.c execution
-FAIL: gcc.c-torture/execute/string-opt-6.c execution
-FAIL: gcc.c-torture/execute/string-opt-7.c execution
-FAIL: gcc.c-torture/execute/struct-ret-1.c compilation
-FAIL: gcc.c-torture/execute/va-arg-22.c execution
-FAIL: gcc.c-torture/unsorted/structret.c
OK to apply?
Kazu Hirata
2003-12-21 Kazu Hirata <kazu@cs.umass.edu>
* config/m32r/m32r.c (m32r_expand_block_move): Call
gen_movestrsi_internal with two more arguments.
(m32r_output_block_move): Adjust operand numbers.
Properly update the source and destination pointers.
* config/m32r/m32r.md (movstrsi_internal): Use 'r' instead of
'r+'. Change the set detinations to match_operand.
Index: m32r.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/m32r/m32r.c,v
retrieving revision 1.76
diff -c -r1.76 m32r.c
*** m32r.c 17 Dec 2003 03:30:19 -0000 1.76
--- m32r.c 22 Dec 2003 16:23:12 -0000
***************
*** 2656,2661 ****
--- 2656,2663 ----
rtx final_src = NULL_RTX;
rtx at_a_time = GEN_INT (MAX_MOVE_BYTES);
rtx rounded_total = GEN_INT (bytes);
+ rtx new_dst_reg = gen_reg_rtx (SImode);
+ rtx new_src_reg = gen_reg_rtx (SImode);
/* If we are going to have to perform this loop more than
once, then generate a label and compute the address the
***************
*** 2681,2687 ****
to the word after the end of the source block, and dst_reg to point
to the last word of the destination block, provided that the block
is MAX_MOVE_BYTES long. */
! emit_insn (gen_movstrsi_internal (dst_reg, src_reg, at_a_time));
emit_insn (gen_addsi3 (dst_reg, dst_reg, GEN_INT (4)));
if (bytes > MAX_MOVE_BYTES)
--- 2683,2692 ----
to the word after the end of the source block, and dst_reg to point
to the last word of the destination block, provided that the block
is MAX_MOVE_BYTES long. */
! emit_insn (gen_movstrsi_internal (dst_reg, src_reg, at_a_time,
! new_dst_reg, new_src_reg));
! emit_move_insn (dst_reg, new_dst_reg);
! emit_move_insn (src_reg, new_src_reg);
emit_insn (gen_addsi3 (dst_reg, dst_reg, GEN_INT (4)));
if (bytes > MAX_MOVE_BYTES)
***************
*** 2692,2698 ****
}
if (leftover)
! emit_insn (gen_movstrsi_internal (dst_reg, src_reg, GEN_INT (leftover)));
}
--- 2697,2705 ----
}
if (leftover)
! emit_insn (gen_movstrsi_internal (dst_reg, src_reg, GEN_INT (leftover),
! gen_reg_rtx (SImode),
! gen_reg_rtx (SImode)));
}
***************
*** 2728,2744 ****
{
if (first_time)
{
! output_asm_insn ("ld\t%3, %p1", operands);
! output_asm_insn ("ld\t%4, %p1", operands);
! output_asm_insn ("st\t%3, @%0", operands);
! output_asm_insn ("st\t%4, %s0", operands);
}
else
{
! output_asm_insn ("ld\t%3, %p1", operands);
! output_asm_insn ("ld\t%4, %p1", operands);
! output_asm_insn ("st\t%3, %s0", operands);
! output_asm_insn ("st\t%4, %s0", operands);
}
bytes -= 8;
--- 2735,2751 ----
{
if (first_time)
{
! output_asm_insn ("ld\t%5, %p1", operands);
! output_asm_insn ("ld\t%6, %p1", operands);
! output_asm_insn ("st\t%5, @%0", operands);
! output_asm_insn ("st\t%6, %s0", operands);
}
else
{
! output_asm_insn ("ld\t%5, %p1", operands);
! output_asm_insn ("ld\t%6, %p1", operands);
! output_asm_insn ("st\t%5, %s0", operands);
! output_asm_insn ("st\t%6, %s0", operands);
}
bytes -= 8;
***************
*** 2748,2762 ****
if (bytes > 4)
got_extra = 1;
! output_asm_insn ("ld\t%3, %p1", operands);
if (got_extra)
! output_asm_insn ("ld\t%4, %p1", operands);
if (first_time)
! output_asm_insn ("st\t%3, @%0", operands);
else
! output_asm_insn ("st\t%3, %s0", operands);
bytes -= 4;
}
--- 2755,2769 ----
if (bytes > 4)
got_extra = 1;
! output_asm_insn ("ld\t%5, %p1", operands);
if (got_extra)
! output_asm_insn ("ld\t%6, %p1", operands);
if (first_time)
! output_asm_insn ("st\t%5, @%0", operands);
else
! output_asm_insn ("st\t%5, %s0", operands);
bytes -= 4;
}
***************
*** 2768,2787 ****
valid memory [since we don't get called if things aren't properly
aligned]. */
int dst_offset = first_time ? 0 : 4;
int last_shift;
rtx my_operands[3];
/* If got_extra is true then we have already loaded
the next word as part of loading and storing the previous word. */
if (! got_extra)
! output_asm_insn ("ld\t%4, @%1", operands);
if (bytes >= 2)
{
bytes -= 2;
! output_asm_insn ("sra3\t%3, %4, #16", operands);
! my_operands[0] = operands[3];
my_operands[1] = GEN_INT (dst_offset);
my_operands[2] = operands[0];
output_asm_insn ("sth\t%0, @(%1,%2)", my_operands);
--- 2775,2799 ----
valid memory [since we don't get called if things aren't properly
aligned]. */
int dst_offset = first_time ? 0 : 4;
+ /* The amount of increment we have to make to the
+ destination pointer. */
+ int dst_inc_amount = dst_offset + bytes - 4;
+ /* The same for the source pointer. */
+ int src_inc_amount = bytes;
int last_shift;
rtx my_operands[3];
/* If got_extra is true then we have already loaded
the next word as part of loading and storing the previous word. */
if (! got_extra)
! output_asm_insn ("ld\t%6, @%1", operands);
if (bytes >= 2)
{
bytes -= 2;
! output_asm_insn ("sra3\t%5, %6, #16", operands);
! my_operands[0] = operands[5];
my_operands[1] = GEN_INT (dst_offset);
my_operands[2] = operands[0];
output_asm_insn ("sth\t%0, @(%1,%2)", my_operands);
***************
*** 2802,2814 ****
if (bytes > 0)
{
! my_operands[0] = operands[4];
my_operands[1] = GEN_INT (last_shift);
output_asm_insn ("srai\t%0, #%1", my_operands);
! my_operands[0] = operands[4];
my_operands[1] = GEN_INT (dst_offset);
my_operands[2] = operands[0];
output_asm_insn ("stb\t%0, @(%1,%2)", my_operands);
}
bytes = 0;
--- 2814,2848 ----
if (bytes > 0)
{
! my_operands[0] = operands[6];
my_operands[1] = GEN_INT (last_shift);
output_asm_insn ("srai\t%0, #%1", my_operands);
! my_operands[0] = operands[6];
my_operands[1] = GEN_INT (dst_offset);
my_operands[2] = operands[0];
output_asm_insn ("stb\t%0, @(%1,%2)", my_operands);
+ }
+
+ /* Update the destination pointer if needed. We have to do
+ this so that the patterns matches what we output in this
+ function. */
+ if (dst_inc_amount
+ && !find_reg_note (insn, REG_UNUSED, operands[0]))
+ {
+ my_operands[0] = operands[0];
+ my_operands[1] = GEN_INT (dst_inc_amount);
+ output_asm_insn ("addi\t%0, #%1", my_operands);
+ }
+
+ /* Update the source pointer if needed. We have to do this
+ so that the patterns matches what we output in this
+ function. */
+ if (src_inc_amount
+ && !find_reg_note (insn, REG_UNUSED, operands[1]))
+ {
+ my_operands[0] = operands[1];
+ my_operands[1] = GEN_INT (src_inc_amount);
+ output_asm_insn ("addi\t%0, #%1", my_operands);
}
bytes = 0;
Index: m32r.md
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/m32r/m32r.md,v
retrieving revision 1.33
diff -c -r1.33 m32r.md
*** m32r.md 17 Dec 2003 03:30:19 -0000 1.33
--- m32r.md 22 Dec 2003 16:23:13 -0000
***************
*** 2570,2582 ****
;; Insn generated by block moves
(define_insn "movstrsi_internal"
! [(set (mem:BLK (match_operand:SI 0 "register_operand" "+r")) ;; destination
! (mem:BLK (match_operand:SI 1 "register_operand" "+r"))) ;; source
(use (match_operand:SI 2 "m32r_block_immediate_operand" "J"));; # bytes to move
! (set (match_dup 0) (plus:SI (match_dup 0) (minus:SI (match_dup 2) (const_int 4))))
! (set (match_dup 1) (plus:SI (match_dup 1) (match_dup 2)))
! (clobber (match_scratch:SI 3 "=&r")) ;; temp 1
! (clobber (match_scratch:SI 4 "=&r"))] ;; temp 2
""
"* m32r_output_block_move (insn, operands); return \"\"; "
[(set_attr "type" "store8")
--- 2570,2586 ----
;; Insn generated by block moves
(define_insn "movstrsi_internal"
! [(set (mem:BLK (match_operand:SI 0 "register_operand" "r")) ;; destination
! (mem:BLK (match_operand:SI 1 "register_operand" "r"))) ;; source
(use (match_operand:SI 2 "m32r_block_immediate_operand" "J"));; # bytes to move
! (set (match_operand:SI 3 "register_operand" "=0")
! (plus:SI (match_dup 0)
! (minus (match_dup 2) (const_int 4))))
! (set (match_operand:SI 4 "register_operand" "=1")
! (plus:SI (match_dup 1)
! (match_dup 2)))
! (clobber (match_scratch:SI 5 "=&r")) ;; temp1
! (clobber (match_scratch:SI 6 "=&r"))] ;; temp2
""
"* m32r_output_block_move (insn, operands); return \"\"; "
[(set_attr "type" "store8")