diff mbox

[AVR] : Fix PR50063 GCC does not support FP = SP

Message ID 4F43CF99.2040706@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay Feb. 21, 2012, 5:08 p.m. UTC
Jakub Jelinek wrote:
> On Tue, Feb 21, 2012 at 12:38:48PM +0100, Georg-Johann Lay wrote:
>> However, if all insns that are involved in SP/FP manipulation get the
>> RTX_FRAME_RELATED_P, almost every test case that has -g crashes with this
>> dwarf-ICE.
> 
> My suggestion actually wasn't to use unspec for reading from sp, but instead
> to use it on the insn which sets sp from hfp, i.e. instead of that
>    (insn 47 46 48 2 (set (reg/f:HI 32 __SP_L__)
>            (reg/f:HI 28 r28)) pr50063.c:9 -1
>         (nil))
> in the testcase use:
>    (insn 47 46 48 2 (set (reg/f:HI 32 __SP_L__)
>            (unspec [(reg/f:HI 28 r28)] UNSPEC_SET_SP_FROM_FP)) pr50063.c:9 -1
>         (nil))
> or so.  You should be able to use REG_FRAME_RELATED_EXPR with the old set
> if needed (though, the insn strangely isn't RTX_FRAME_RELATED_P, preexisting
> bug?).  Just to hide from cselib/alias code that you are setting hfp from
> sp, then adjusting hfp and then setting sp back from it.  With the unspec
> it will just not know what value sp has there.
> 
> 	Jakub


Thanks, it works now :-)) Here is the patch for review now.

It passes the testsuite and fixed several execution fails because of
assumptions in DSE.  Besides the normal testsuite run, it passes also a run
with -maccumulate-args.

Ok for trunk?

Johann

	PR rtl-optimization/50063
	* config/avr/avr.md (movhi_sp_r): Handle -1 (unknown IRQ state)
	and 2 (8-bit SP) in operand 2.
	* config/avr/avr.c (avr_prologue_setup_frame): Adjust prologue
	setup to use movhi_sp_r instead of vanilla move to write SP.
	Adjust REG_CFA notes to superseed unspec.
	(expand_epilogue): Adjust epilogue setup to use read_sp instead
	of vanilla move.
	As function body might contain CLI or SEI: Use irq_state 0 (IRQ
	known to be off) only with TARGET_NO_INTERRUPTS. Never use
	irq_state 1 (IRQ known to be on) here.

Comments

Richard Henderson Feb. 21, 2012, 5:22 p.m. UTC | #1
On 02/21/12 09:08, Georg-Johann Lay wrote:
> 	PR rtl-optimization/50063
> 	* config/avr/avr.md (movhi_sp_r): Handle -1 (unknown IRQ state)
> 	and 2 (8-bit SP) in operand 2.
> 	* config/avr/avr.c (avr_prologue_setup_frame): Adjust prologue
> 	setup to use movhi_sp_r instead of vanilla move to write SP.
> 	Adjust REG_CFA notes to superseed unspec.
> 	(expand_epilogue): Adjust epilogue setup to use read_sp instead
> 	of vanilla move.
> 	As function body might contain CLI or SEI: Use irq_state 0 (IRQ
> 	known to be off) only with TARGET_NO_INTERRUPTS. Never use
> 	irq_state 1 (IRQ known to be on) here.

The CFA bits in avr_prologue_setup_frame look good.
I'll let Denis or Eric review the movhi_sp_r change.


r~
Denis Chertykov Feb. 21, 2012, 7:18 p.m. UTC | #2
2012/2/21 Richard Henderson <rth@redhat.com>:
> On 02/21/12 09:08, Georg-Johann Lay wrote:
>>       PR rtl-optimization/50063
>>       * config/avr/avr.md (movhi_sp_r): Handle -1 (unknown IRQ state)
>>       and 2 (8-bit SP) in operand 2.
>>       * config/avr/avr.c (avr_prologue_setup_frame): Adjust prologue
>>       setup to use movhi_sp_r instead of vanilla move to write SP.
>>       Adjust REG_CFA notes to superseed unspec.
>>       (expand_epilogue): Adjust epilogue setup to use read_sp instead
>>       of vanilla move.
>>       As function body might contain CLI or SEI: Use irq_state 0 (IRQ
>>       known to be off) only with TARGET_NO_INTERRUPTS. Never use
>>       irq_state 1 (IRQ known to be on) here.
>
> The CFA bits in avr_prologue_setup_frame look good.
> I'll let Denis or Eric review the movhi_sp_r change.
>

Approved.

Denis.
diff mbox

Patch

Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(revision 184386)
+++ config/avr/avr.md	(working copy)
@@ -583,23 +583,26 @@  (define_peephole2
 
 ;; Move register $1 to the Stack Pointer register SP.
 ;; This insn is emit during function prologue/epilogue generation.
-;;    $2 = 0: We know that IRQs are off
-;;    $2 = 1: We know that IRQs are on
-;; Remaining cases when the state of the I-Flag is unknown are
-;; handled by generic movhi insn.
+;;    $2 =  0: We know that IRQs are off
+;;    $2 =  1: We know that IRQs are on
+;;    $2 =  2: SP has 8 bits only, IRQ state does not matter
+;;    $2 = -1: We don't know anything about IRQ on/off
+;; Always write SP via unspec, see PR50063
 
 (define_insn "movhi_sp_r"
-  [(set (match_operand:HI 0 "stack_register_operand"                "=q,q,q")
-        (unspec_volatile:HI [(match_operand:HI 1 "register_operand"  "r,r,r")
-                             (match_operand:HI 2 "const_int_operand" "L,P,LP")]
+  [(set (match_operand:HI 0 "stack_register_operand"                "=q,q,q,q,q")
+        (unspec_volatile:HI [(match_operand:HI 1 "register_operand"  "r,r,r,r,r")
+                             (match_operand:HI 2 "const_int_operand" "L,P,N,K,LPN")]
                             UNSPECV_WRITE_SP))]
-  "!AVR_HAVE_8BIT_SP"
+  ""
   "@
-	out __SP_H__,%B1\;out __SP_L__,%A1
-	cli\;out __SP_H__,%B1\;sei\;out __SP_L__,%A1
-	out __SP_L__,%A1\;out __SP_H__,%B1"
-  [(set_attr "length" "2,4,2")
-   (set_attr "isa" "no_xmega,no_xmega,xmega")
+	out %B0,%B1\;out %A0,%A1
+	cli\;out %B0,%B1\;sei\;out %A0,%A1
+	in __tmp_reg__,__SREG__\;cli\;out %B0,%B1\;out __SREG__,__tmp_reg__\;out %A0,%A1
+	out %A0,%A1
+	out %A0,%A1\;out %B0,%B1"
+  [(set_attr "length" "2,4,5,1,2")
+   (set_attr "isa" "no_xmega,no_xmega,no_xmega,*,xmega")
    (set_attr "cc" "none")])
 
 (define_peephole2
Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 184386)
+++ config/avr/avr.c	(working copy)
@@ -1062,8 +1062,8 @@  avr_prologue_setup_frame (HOST_WIDE_INT
               !frame_pointer_needed can only occur if the function is not a
               leaf function and thus X has already been saved.  */
               
+          int irq_state = -1;
           rtx fp_plus_insns, fp, my_fp;
-          rtx sp_minus_size = plus_constant (stack_pointer_rtx, -size);
 
           gcc_assert (frame_pointer_needed
                       || !isr_p
@@ -1076,7 +1076,7 @@  avr_prologue_setup_frame (HOST_WIDE_INT
           if (AVR_HAVE_8BIT_SP)
             {
               /* The high byte (r29) does not change:
-                 Prefer SUBI (1 cycle) over ABIW (2 cycles, same size).  */
+                 Prefer SUBI (1 cycle) over SBIW (2 cycles, same size).  */
 
               my_fp = all_regs_rtx[FRAME_POINTER_REGNUM];
             }
@@ -1092,43 +1092,50 @@  avr_prologue_setup_frame (HOST_WIDE_INT
              the frame pointer subtraction is done.  */
           
           insn = emit_move_insn (fp, stack_pointer_rtx);
-          if (!frame_pointer_needed)
-            RTX_FRAME_RELATED_P (insn) = 1;
+          if (frame_pointer_needed)
+            {
+              RTX_FRAME_RELATED_P (insn) = 1;
+              add_reg_note (insn, REG_CFA_ADJUST_CFA,
+                            gen_rtx_SET (VOIDmode, fp, stack_pointer_rtx));
+            }
 
           insn = emit_move_insn (my_fp, plus_constant (my_fp, -size));
-          RTX_FRAME_RELATED_P (insn) = 1;
-          
           if (frame_pointer_needed)
             {
+              RTX_FRAME_RELATED_P (insn) = 1;
               add_reg_note (insn, REG_CFA_ADJUST_CFA,
-                            gen_rtx_SET (VOIDmode, fp, sp_minus_size));
+                            gen_rtx_SET (VOIDmode, fp,
+                                         plus_constant (fp, -size)));
             }
           
           /* Copy to stack pointer.  Note that since we've already
              changed the CFA to the frame pointer this operation
-             need not be annotated if frame pointer is needed.  */
-              
-          if (AVR_HAVE_8BIT_SP || AVR_XMEGA)
-            {
-              insn = emit_move_insn (stack_pointer_rtx, fp);
-            }
-          else if (TARGET_NO_INTERRUPTS 
-                   || isr_p
-                   || cfun->machine->is_OS_main)
-            {
-              rtx irqs_are_on = GEN_INT (!!cfun->machine->is_interrupt);
-              
-              insn = emit_insn (gen_movhi_sp_r (stack_pointer_rtx,
-                                                fp, irqs_are_on));
-            }
-          else
-            {
-              insn = emit_move_insn (stack_pointer_rtx, fp);
-            }
+             need not be annotated if frame pointer is needed.
+             Always move through unspec, see PR50063.
+             For meaning of irq_state see movhi_sp_r insn.  */
+
+          if (cfun->machine->is_interrupt)
+            irq_state = 1;
+
+          if (TARGET_NO_INTERRUPTS
+              || cfun->machine->is_signal
+              || cfun->machine->is_OS_main)
+            irq_state = 0;
 
-          if (!frame_pointer_needed)
-            RTX_FRAME_RELATED_P (insn) = 1;
+          if (AVR_HAVE_8BIT_SP)
+            irq_state = 2;
 
+          insn = emit_insn (gen_movhi_sp_r (stack_pointer_rtx,
+                                            fp, GEN_INT (irq_state)));
+          if (!frame_pointer_needed)
+            {
+              RTX_FRAME_RELATED_P (insn) = 1;
+              add_reg_note (insn, REG_CFA_ADJUST_CFA,
+                            gen_rtx_SET (VOIDmode, stack_pointer_rtx,
+                                         plus_constant (stack_pointer_rtx,
+                                                        -size)));
+            }
+          
           fp_plus_insns = get_insns ();
           end_sequence ();
           
@@ -1143,9 +1150,13 @@  avr_prologue_setup_frame (HOST_WIDE_INT
               
               start_sequence ();
 
-              insn = emit_move_insn (stack_pointer_rtx, sp_minus_size);
+              insn = emit_move_insn (stack_pointer_rtx,
+                                     plus_constant (stack_pointer_rtx, -size));
               RTX_FRAME_RELATED_P (insn) = 1;
-
+              add_reg_note (insn, REG_CFA_ADJUST_CFA,
+                            gen_rtx_SET (VOIDmode, stack_pointer_rtx,
+                                         plus_constant (stack_pointer_rtx,
+                                                        -size)));
               if (frame_pointer_needed)
                 {
                   insn = emit_move_insn (fp, stack_pointer_rtx);
@@ -1376,7 +1387,8 @@  expand_epilogue (bool sibcall_p)
   if (size)
     {
       /* Try two methods to adjust stack and select shortest.  */
-          
+
+      int irq_state = -1;
       rtx fp, my_fp;
       rtx fp_plus_insns;
 
@@ -1406,23 +1418,15 @@  expand_epilogue (bool sibcall_p)
       emit_move_insn (my_fp, plus_constant (my_fp, size));
 
       /* Copy to stack pointer.  */
-              
-      if (AVR_HAVE_8BIT_SP || AVR_XMEGA)
-        {
-          emit_move_insn (stack_pointer_rtx, fp);
-        }
-      else if (TARGET_NO_INTERRUPTS 
-               || isr_p
-               || cfun->machine->is_OS_main)
-        {
-          rtx irqs_are_on = GEN_INT (!!cfun->machine->is_interrupt);
-          
-          emit_insn (gen_movhi_sp_r (stack_pointer_rtx, fp, irqs_are_on));
-        }
-      else
-        {
-          emit_move_insn (stack_pointer_rtx, fp);
-        }
+
+      if (TARGET_NO_INTERRUPTS)
+        irq_state = 0;
+
+      if (AVR_HAVE_8BIT_SP)
+        irq_state = 2;
+
+      emit_insn (gen_movhi_sp_r (stack_pointer_rtx, fp,
+                                 GEN_INT (irq_state)));
 
       fp_plus_insns = get_insns ();
       end_sequence ();