[AVR] Fix/hack around spill fail ICE PR52148

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.

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.

2012/2/24 Georg-Johann Lay <avr@gjlay.de>:
> 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

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

2012/2/25 Georg-Johann Lay <avr@gjlay.de>:
[...]
> 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")

Thanks for explanation.
This technique similar to mulhi3, mulhi3_call ...
IMHO this is a better solution then very strict constraint 'x' or 'y' or 'z'.
I.e. better to have (at least from reload's point of view):
mov r26, "r"
mov r27, "r"
r30,r31 = do-something-with r26, r27
mov "r", r30
mov "r", r31
Than:
"z" = do-something-with "x"

Denis.