This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Ping #1: [Patch,AVR] Fix/hack around spill fail ICE PR52148
Denis Chertykov schrieb:
2012/2/24 Georg-Johann Lay:
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00956.html
Georg-Johann Lay wrote:
Spill failure PR52148 occurs for movmem insn that allocates 2 of AVR's 3
pointer registers. Register allocator is at it's limits and the patch tries to
cure the situation by replacing
(match_operand:HI 0 "register_operand" "x")
with explicit
(reg:HI REG_X)
and similar for Z Register classes "x" and "z" contain only one HI register.
This PR and PR50925 show that register allocator has some problems.
Even though this patch is not a fix of the root cause, it allows the PR's test
case to compile.
Anyways, the patch simplifies the backend and replaces an insn with 11(!)
operands with an insn with only 2 operands so that the patch is improvement of
the backend.
The hard registers are already known at expand time so there is no need for
match_operands.
Passes without regression.
Ok for trunk?
Johann
PR target/52148
* config/avr/avr.md (movmem_<mode>): Replace match_operand that
match only one single hard register with respective hard reg rtx.
(movmemx_<mode>): Ditto.
* config/avr/avr.c (avr_emit_movmemhi): Adapt expanding to new
insn anatomy of movmem[x]_<mode>.
(avr_out_movmem): Same for printing assembler and operand usage.
Approved.
Denis.
PS: I have not understood this patch from first look
In 4.6, movmemhi expands the copy loop explicitly:
/* Move one byte into scratch and inc pointer. */
emit_move_insn (tmp_reg_rtx, gen_rtx_MEM (QImode, addr1));
emit_move_insn (addr1, gen_rtx_PLUS (Pmode, addr1, const1_rtx));
/* Move to mem and inc pointer. */
emit_move_insn (gen_rtx_MEM (QImode, addr0), tmp_reg_rtx);
emit_move_insn (addr0, gen_rtx_PLUS (Pmode, addr0, const1_rtx));
tmp_reg is fixed and used implicitly by insn templates; the
assertion is that the lifetime of tmp_reg does never
extend over one insn.
The 4.6 movmemhi violates that assertion, and in 4.7 I saw situations
where reload generates insns that use (i.e. clobber) tmp_reg so that
explicit expanding is wrong. This happend together with movmemhi on
address spaces:
/* FIXME: Register allocator does a bad job and might spill
address register(s) inside the loop leading to additional
move instruction to/from stack which could clobber tmp_reg.
Thus, do *not* emit load and store as seperate insns.
Instead, we perform the copy by means of one
monolithic insn. */
To fix it, I put all the movmemhi stuff into an unspec insn, but that
insn is not nice: The address-space version has 11 operands to
describe the insns action and the ordinary version 7 operands:
;; $0, $4 : & dest (REG_X)
;; $1, $5 : & src (REG_Z)
;; $2 : Address Space
;; $3, $7 : Loop register
;; $6 : Scratch register
;; "movmem_qi"
;; "movmem_hi"
(define_insn "movmem_<mode>"
[(set (mem:BLK (match_operand:HI 0 "register_operand" "x"))
(mem:BLK (match_operand:HI 1 "register_operand" "z")))
(unspec [(match_operand:QI 2 "const_int_operand" "n")]
UNSPEC_MOVMEM)
(use (match_operand:QIHI 3 "register_operand" "<MOVMEM_r_d>"))
(clobber (match_operand:HI 4 "register_operand" "=0"))
(clobber (match_operand:HI 5 "register_operand" "=1"))
(clobber (match_operand:QI 6 "register_operand" "=&r"))
(clobber (match_operand:QIHI 7 "register_operand" "=3"))]
""
{
return avr_out_movmem (insn, operands, NULL);
}
[(set_attr "adjust_len" "movmem")
(set_attr "cc" "clobber")])
Even more issues are that the expand machinery expects a specific style
for movmem patterns (or was it clrstrhi?) and that with "free" register
assignments, i.e. with constrints "b" for the addresses,
I saw spill fails.
Outcome was the pattern above, that only allows one specific allocation
of the pointer registers involved; similar for the address space version:
;; Ditto and
;; $3, $7 : Loop register = R24
;; $8, $9 : hh8 (& src) = R23
;; $10 : RAMPZ_ADDR
;; "movmemx_qi"
;; "movmemx_hi"
(define_insn "movmemx_<mode>"
[(set (mem:BLK (match_operand:HI 0 "register_operand" "x"))
(mem:BLK (lo_sum:PSI (match_operand:QI 8 "register_operand" "r")
(match_operand:HI 1 "register_operand" "z"))))
(unspec [(match_operand:QI 2 "const_int_operand""n")]
UNSPEC_MOVMEM)
(use (match_operand:QIHI 3 "register_operand" "w"))
(clobber (match_operand:HI 4 "register_operand" "=0"))
(clobber (match_operand:HI 5 "register_operand" "=1"))
(clobber (match_operand:QI 6 "register_operand" "=&r"))
(clobber (match_operand:HI 7 "register_operand" "=3"))
(clobber (match_operand:QI 9 "register_operand" "=8"))
(clobber (mem:QI (match_operand:QI 10 "io_address_operand" "n")))]
""
"%~call __movmemx_<mode>"
[(set_attr "type" "xcall")
(set_attr "cc" "clobber")])
With these patterns the test suite passed but are were more FIXMEs
in the source:
/* FIXME: Register allocator might come up with spill fails
if it is left on its own.
Thus, we allocate the pointer registers by hand:
Z = source address
X = destination address */
However, there is this PR now and I used it to clean up the code.
As the intention is to hard-assign the pointer registers, anyway,
there is no need to have operands for them. Obviously, reload tries
to spill some of the addresses which is no good idea.
As said, this is not a fix of the root cause of the spill fail
like PR50925, which is still open as you know.
But this small patch removes the spill fail for the test case at
least, and in itself it is a code clean-up that I preferred
over the original version.
The pattern for the address spaces is now as simple as
;; $0 : Address Space
;; $1 : RAMPZ RAM address
;; R24 : #bytes and loop register
;; R23:Z : 24-bit source address
;; R26 : 16-bit destination address
;; "movmemx_qi"
;; "movmemx_hi"
(define_insn "movmemx_<mode>"
[(set (mem:BLK (reg:HI REG_X))
(mem:BLK (lo_sum:PSI (reg:QI 23)
(reg:HI REG_Z))))
(unspec [(match_operand:QI 0 "const_int_operand" "n")]
UNSPEC_MOVMEM)
(use (reg:QIHI 24))
(clobber (reg:HI REG_X))
(clobber (reg:HI REG_Z))
(clobber (reg:QI LPM_REGNO))
(clobber (reg:HI 24))
(clobber (reg:QI 23))
(clobber (mem:QI (match_operand:QI 1 "io_address_operand" "n")))]
""
"%~call __movmemx_<mode>"
[(set_attr "type" "xcall")
Johann