[patch, avr] Fix ICE PR64452 pushing eliminated rtxes

[patch, avr] Fix ICE PR64452 pushing eliminated rtxes

[Georg-Johann Lay]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Byte-wise pushing virtual regs like arg pointer migth result in patterns like

  (set (mem:QI (post_dec:HI (reg:HI 32 SP)))
       (subreg:QI (plus:HI (reg:HI 28)
                           (const_int 17)) 0))

after elimination.

Attached patch uses new pushhi1_insn to push virtuals in HImode so that 
expressions like in subreg_reg from above can be reloaded.

Ok to commit ?

Johann

	PR target/64452

	* config/avr/avr.md (pushhi_insn): New insn.
	(push<mode>1): Push virtual regs in one chunk using pushhi1_insn.

[-- Attachment #2: pr64452.diff --]
[-- Type: text/x-patch, Size: 2671 bytes --]

Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(revision 220738)
+++ config/avr/avr.md	(working copy)
@@ -371,6 +371,13 @@ (define_insn "push<mode>1"
 	push __zero_reg__"
   [(set_attr "length" "1,1")])
 
+(define_insn "pushhi1_insn"
+  [(set (mem:HI (post_dec:HI (reg:HI REG_SP)))
+        (match_operand:HI 0 "register_operand" "r"))]
+  ""
+  "push %B0\;push %A0"
+  [(set_attr "length" "2")])
+
 ;; All modes for a multi-byte push.  We must include complex modes here too,
 ;; lest emit_single_push_insn "helpfully" create the auto-inc itself.
 (define_mode_iterator MPUSH
@@ -386,17 +393,42 @@ (define_expand "push<mode>1"
   [(match_operand:MPUSH 0 "" "")]
   ""
   {
-    int i;
-
-    // Avoid (subreg (mem)) for non-generic address spaces below.  Because
-    // of the poor addressing capabilities of these spaces it's better to
-    // load them in one chunk.  And it avoids PR61443.
-
     if (MEM_P (operands[0])
         && !ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (operands[0])))
-      operands[0] = copy_to_mode_reg (<MODE>mode, operands[0]);
+      {
+        // Avoid (subreg (mem)) for non-generic address spaces.  Because
+        // of the poor addressing capabilities of these spaces it's better to
+        // load them in one chunk.  And it avoids PR61443.
+
+        operands[0] = copy_to_mode_reg (<MODE>mode, operands[0]);
+      }
+    else if (REG_P (operands[0])
+             && IN_RANGE (REGNO (operands[0]), FIRST_VIRTUAL_REGISTER,
+                          LAST_VIRTUAL_REGISTER))
+      {
+        // Byte-wise pushing of virtual regs might result in something like
+        //
+        //     (set (mem:QI (post_dec:HI (reg:HI 32 SP)))
+        //          (subreg:QI (plus:HI (reg:HI 28)
+        //                              (const_int 17)) 0))
+        //
+        // after elimination.  This cannot be handled by reload, cf. PR64452.
+        // Reload virtuals in one chunk.  That way it's possible to reload
+        // above situation and finally
+        //
+        //    (set (reg:HI **)
+        //         (const_int 17))
+        //    (set (reg:HI **)
+        //         (plus:HI (reg:HI **)
+        //                  (reg:HI 28)))
+        //    (set (mem:HI (post_dec:HI (reg:HI 32 SP))
+        //         (reg:HI **)))
+ 
+        emit_insn (gen_pushhi1_insn (operands[0]));
+        DONE;
+      }
 
-    for (i = GET_MODE_SIZE (<MODE>mode) - 1; i >= 0; --i)
+    for (int i = GET_MODE_SIZE (<MODE>mode) - 1; i >= 0; --i)
       {
         rtx part = simplify_gen_subreg (QImode, operands[0], <MODE>mode, i);
         if (part != const0_rtx)

Re: [patch, avr] Fix ICE PR64452 pushing eliminated rtxes

[Denis Chertykov]

2015-02-17 14:12 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
> Byte-wise pushing virtual regs like arg pointer migth result in patterns
> like
>
>  (set (mem:QI (post_dec:HI (reg:HI 32 SP)))
>       (subreg:QI (plus:HI (reg:HI 28)
>                           (const_int 17)) 0))
>
> after elimination.
>
> Attached patch uses new pushhi1_insn to push virtuals in HImode so that
> expressions like in subreg_reg from above can be reloaded.
>
> Ok to commit ?
>
> Johann
>
>         PR target/64452
>
>         * config/avr/avr.md (pushhi_insn): New insn.
>         (push<mode>1): Push virtual regs in one chunk using pushhi1_insn.

Approved.
(But I'm worry about this because it's reload related problem and it
can have a side effect)

Denis.

Re: [patch, avr] Fix ICE PR64452 pushing eliminated rtxes

[Georg-Johann Lay]

Am 02/17/2015 um 03:34 PM schrieb Denis Chertykov:
> 2015-02-17 14:12 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
>> Byte-wise pushing virtual regs like arg pointer migth result in patterns
>> like
>>
>>   (set (mem:QI (post_dec:HI (reg:HI 32 SP)))
>>        (subreg:QI (plus:HI (reg:HI 28)
>>                            (const_int 17)) 0))
>>
>> after elimination.
>>
>> Attached patch uses new pushhi1_insn to push virtuals in HImode so that
>> expressions like in subreg_reg from above can be reloaded.
>>
>> Ok to commit ?
>>
>> Johann
>>
>>          PR target/64452
>>
>>          * config/avr/avr.md (pushhi_insn): New insn.
>>          (push<mode>1): Push virtual regs in one chunk using pushhi1_insn.
>
> Approved.
> (But I'm worry about this because it's reload related problem and it
> can have a side effect)
>
> Denis.

So you have a superior solution in mind?

What side effects specifically?

Currently the side effect is that reload gets simpler expressions and hence 
does not ICE.  There isn't even an insn that can push complex (plus rtx in this 
case) expressions or subregs thereof.  Even if there were such insns I don't 
think reload is supposed to handle them.

The current implementation of push<mode>1 assumes that all RTXes which ever 
appear in a push can be decomposed into subregs and these can be simplified to 
some of the push insns, i.e. the push operand simplifies to REG or CONST0_RTX. 
  The subreg above, however, cannot be simplified to anything reload can handle 
and does not match an insn.  And supplying such an insn is pointless because 
that insn would need a scratch and hence require secondary reloads...

plus rtxes are special as they might be produced by reload (R28 above is 
(hard_)frame_pointer).  For similar reason there are two addhi3 insns (one 
without scratch to accommodate reload and one generic with scratch for better 
performance.)


Johann

Re: [patch, avr] Fix ICE PR64452 pushing eliminated rtxes

[Denis Chertykov]

2015-02-18 14:59 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
> Am 02/17/2015 um 03:34 PM schrieb Denis Chertykov:
>
>> 2015-02-17 14:12 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
>>>
>>> Byte-wise pushing virtual regs like arg pointer migth result in patterns
>>> like
>>>
>>>   (set (mem:QI (post_dec:HI (reg:HI 32 SP)))
>>>        (subreg:QI (plus:HI (reg:HI 28)
>>>                            (const_int 17)) 0))
>>>
>>> after elimination.
>>>
>>> Attached patch uses new pushhi1_insn to push virtuals in HImode so that
>>> expressions like in subreg_reg from above can be reloaded.
>>>
>>> Ok to commit ?
>>>
>>> Johann
>>>
>>>          PR target/64452
>>>
>>>          * config/avr/avr.md (pushhi_insn): New insn.
>>>          (push<mode>1): Push virtual regs in one chunk using
>>> pushhi1_insn.
>>
>>
>> Approved.
>> (But I'm worry about this because it's reload related problem and it
>> can have a side effect)
>>
>> Denis.
>
>
> So you have a superior solution in mind?
>
> What side effects specifically?
>
> Currently the side effect is that reload gets simpler expressions and hence
> does not ICE.  There isn't even an insn that can push complex (plus rtx in
> this case) expressions or subregs thereof.  Even if there were such insns I
> don't think reload is supposed to handle them.
>
> The current implementation of push<mode>1 assumes that all RTXes which ever
> appear in a push can be decomposed into subregs and these can be simplified
> to some of the push insns, i.e. the push operand simplifies to REG or
> CONST0_RTX.  The subreg above, however, cannot be simplified to anything
> reload can handle and does not match an insn.  And supplying such an insn is
> pointless because that insn would need a scratch and hence require secondary
> reloads...
>
> plus rtxes are special as they might be produced by reload (R28 above is
> (hard_)frame_pointer).  For similar reason there are two addhi3 insns (one
> without scratch to accommodate reload and one generic with scratch for
> better performance.)

I don't have any concrete objections.
I'm worried because it's not so easy to predict all possible reloads.
(At least for me)

Denis.