Patchwork [avr] gas support for cfi info

login
register
mail settings
Submitter Richard Henderson
Date Feb. 17, 2011, 7:52 p.m.
Message ID <4D5D7C86.2080804@redhat.com>
Download mbox | patch
Permalink /patch/83478/
State New
Headers show

Comments

Richard Henderson - Feb. 17, 2011, 7:52 p.m.
On 02/17/2011 08:04 AM, Richard Henderson wrote:
>> The other scenario is - how about functions with signals/interrupts?
>> The compiler will give an ICE compiling a function as below:
>>
>> void my_interrupt_handler() __attribute__ (("interrupt"));
> 
> It's a bug in the avr backend; the enable_interrupt insn should not be
> marked as frame-related.  We should fix this separately.

Looking at this closer, it seem that the FRAME_RELATED markers in avr.c
were sprinkled at random without really knowing what is going on.  There
were 2 places where UNSPEC_VOLATILE insns were marked frame related,
somehow expecting the unwinder to do something magical.

The following cleans all that up.  There are a couple of odd points:

  (1) SREG and RAMPZ cannot be annotated as saved in the frame of an
      interrupt function without allocating hard register numbers for
      these registers.  Not to be confused with "real" registers, 
      these need a number in the same way that SP_L and SP_H have
      register numbers.

      I've simply ignored unwind info for these for now.

  (2) At present it's possible to use epilogue_restores without
      having used prologue_saves.  I.e. use epilogue_restores with
      an inline prologue.  The problem being that the inline prologue
      uses an HImode push of REG_Y (i.e. r29 first), whereas the code
      in prologue_saves pushes r28 first and epilogue_restores is
      written to expect that.

      I've changed the inline prologue and epilogue to emit explicit
      byte push/pop for r28/r29 in numerical order to match the 
      external routines.

      This has a side effect that r29 actually receives unwind info.
      The generic unwinder doesn't expect to see multi-register
      saves, and only emits one unwind opcode for each save.


r~
Anitha Boyapati - Feb. 22, 2011, 4:17 p.m.
2011/2/18 Richard Henderson <rth@redhat.com>
>
> On 02/17/2011 08:04 AM, Richard Henderson wrote:
> >> The other scenario is - how about functions with signals/interrupts?
> >> The compiler will give an ICE compiling a function as below:
> >>
> >> void my_interrupt_handler() __attribute__ (("interrupt"));
> >
> > It's a bug in the avr backend; the enable_interrupt insn should not be
> > marked as frame-related.  We should fix this separately.
>
> Looking at this closer, it seem that the FRAME_RELATED markers in avr.c
> were sprinkled at random without really knowing what is going on.

This is quite a clean up! I am yet to reach the changes in expand_epilogue.

>  There
> were 2 places where UNSPEC_VOLATILE insns were marked frame related,
> somehow expecting the unwinder to do something magical.
>

Just to be on same page, these are the 2 places:

1. gen_enable_interrupt()
2. gen_call_prologue_saves()


For the latter, can you  explain why adding reg notes is required?

+      add_reg_note (insn, REG_CFA_ADJUST_CFA,
+		    gen_rtx_SET (VOIDmode,
+				 (frame_pointer_needed
+				  ? frame_pointer_rtx : stack_pointer_rtx),
+				 plus_constant (stack_pointer_rtx,
+						-(size + live_seq))));
+

(The comment does say that this is to describe the effect of
UNSPEC_VOLATILE, but how reg notes help?)


> The following cleans all that up.  There are a couple of odd points:
>
>  (1) SREG and RAMPZ cannot be annotated as saved in the frame of an
>      interrupt function without allocating hard register numbers for
>      these registers.  Not to be confused with "real" registers,
>      these need a number in the same way that SP_L and SP_H have
>      register numbers.
>
>      I've simply ignored unwind info for these for now.
>

In addition to the above, there are atleast 4 more registers RAMPX,
RAMPY, RAMPD and EIND. (Actually the support for these registers is in
the form of patches which aren't upstream yet)


>  (2) At present it's possible to use epilogue_restores without
>      having used prologue_saves.  I.e. use epilogue_restores with
>      an inline prologue.  The problem being that the inline prologue
>      uses an HImode push of REG_Y (i.e. r29 first), whereas the code
>      in prologue_saves pushes r28 first and epilogue_restores is
>      written to expect that.
>

Is this is the line being referred to ?

if (frame_pointer_needed)
  {
...
   /* Push frame pointer.  */
insn = emit_move_insn (pushword, frame_pointer_rtx);


On the whole, when the patch is applied, an ICE is longer generated
during the compilation of interrupt and signal functions.  I am yet to
see the changes due to ARG_POINTER_CFA_OFFSET(FNDECL) definition and
the complete unwind info incase of interrupts.

Anitha
Richard Henderson - Feb. 22, 2011, 5:51 p.m.
On 02/22/2011 08:17 AM, Anitha Boyapati wrote:
> Just to be on same page, these are the 2 places:
> 
> 1. gen_enable_interrupt()
> 2. gen_call_prologue_saves()
> 
> 
> For the latter, can you  explain why adding reg notes is required?
> 
> +      add_reg_note (insn, REG_CFA_ADJUST_CFA,
> +		    gen_rtx_SET (VOIDmode,
> +				 (frame_pointer_needed
> +				  ? frame_pointer_rtx : stack_pointer_rtx),
> +				 plus_constant (stack_pointer_rtx,
> +						-(size + live_seq))));
> +
> 
> (The comment does say that this is to describe the effect of
> UNSPEC_VOLATILE, but how reg notes help?)

The external function call represented by the prologue_saves unspec
saves registers to the stack, and allocates stack space.  Both of
these actions are things we want to describe in unwind info.  The
reg notes I added describe all of the actions performed by the
function call.

>>  (2) At present it's possible to use epilogue_restores without
>>      having used prologue_saves.  I.e. use epilogue_restores with
>>      an inline prologue.  The problem being that the inline prologue
>>      uses an HImode push of REG_Y (i.e. r29 first), whereas the code
>>      in prologue_saves pushes r28 first and epilogue_restores is
>>      written to expect that.
>>
> 
> Is this is the line being referred to ?
> 
> if (frame_pointer_needed)
>   {
> ...
>    /* Push frame pointer.  */
> insn = emit_move_insn (pushword, frame_pointer_rtx);

Yes.


r~

Patch

diff --git a/gcc/config/avr/avr-protos.h b/gcc/config/avr/avr-protos.h
index 4569359..06c9254 100644
--- a/gcc/config/avr/avr-protos.h
+++ b/gcc/config/avr/avr-protos.h
@@ -109,6 +109,7 @@  extern RTX_CODE avr_normalize_condition (RTX_CODE condition);
 extern int compare_eq_p (rtx insn);
 extern void out_shift_with_cnt (const char *templ, rtx insn,
 				rtx operands[], int *len, int t_len);
+extern rtx avr_incoming_return_addr_rtx (void);
 #endif /* RTX_CODE */
 
 #ifdef HAVE_MACHINE_MODES
diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c
index 30e4626..6eaa593 100644
--- a/gcc/config/avr/avr.c
+++ b/gcc/config/avr/avr.c
@@ -534,6 +534,35 @@  get_sequence_length (rtx insns)
   return length;
 }
 
+/*  Implement INCOMING_RETURN_ADDR_RTX.  */
+
+rtx
+avr_incoming_return_addr_rtx (void)
+{
+  /* The return address is at the top of the stack.  Note that the push
+     was via post-decrement, which means the actual address is off by one.  */
+  return gen_frame_mem (HImode, plus_constant (stack_pointer_rtx, 1));
+}
+
+/*  Helper for expand_prologue.  Emit a push of a byte register.  */
+
+static void
+emit_push_byte (unsigned regno, bool frame_related_p)
+{
+  rtx mem, reg, insn;
+
+  mem = gen_rtx_POST_DEC (HImode, stack_pointer_rtx);
+  mem = gen_frame_mem (QImode, mem);
+  reg = gen_rtx_REG (QImode, regno);
+
+  insn = emit_insn (gen_rtx_SET (VOIDmode, mem, reg));
+  if (frame_related_p)
+    RTX_FRAME_RELATED_P (insn) = 1;
+
+  cfun->machine->stack_usage++;
+}
+
+
 /*  Output function prologue.  */
 
 void
@@ -543,11 +572,6 @@  expand_prologue (void)
   HARD_REG_SET set;
   int minimize;
   HOST_WIDE_INT size = get_frame_size();
-  /* Define templates for push instructions.  */
-  rtx pushbyte = gen_rtx_MEM (QImode,
-                  gen_rtx_POST_DEC (HImode, stack_pointer_rtx));
-  rtx pushword = gen_rtx_MEM (HImode,
-                  gen_rtx_POST_DEC (HImode, stack_pointer_rtx));
   rtx insn;
   
   /* Init cfun->machine.  */
@@ -575,46 +599,34 @@  expand_prologue (void)
 
   if (cfun->machine->is_interrupt || cfun->machine->is_signal)
     {
+      /* Enable interrupts.  */
       if (cfun->machine->is_interrupt)
-        {
-          /* Enable interrupts.  */
-          insn = emit_insn (gen_enable_interrupt ());
-          RTX_FRAME_RELATED_P (insn) = 1;
-        }
+	emit_insn (gen_enable_interrupt ());
 	
       /* Push zero reg.  */
-      insn = emit_move_insn (pushbyte, zero_reg_rtx);
-      RTX_FRAME_RELATED_P (insn) = 1;
-      cfun->machine->stack_usage++;
+      emit_push_byte (ZERO_REGNO, true);
 
       /* Push tmp reg.  */
-      insn = emit_move_insn (pushbyte, tmp_reg_rtx);
-      RTX_FRAME_RELATED_P (insn) = 1;
-      cfun->machine->stack_usage++;
+      emit_push_byte (TMP_REGNO, true);
 
       /* Push SREG.  */
-      insn = emit_move_insn (tmp_reg_rtx, 
-                             gen_rtx_MEM (QImode, GEN_INT (SREG_ADDR)));
-      RTX_FRAME_RELATED_P (insn) = 1;
-      insn = emit_move_insn (pushbyte, tmp_reg_rtx);
-      RTX_FRAME_RELATED_P (insn) = 1;
-      cfun->machine->stack_usage++;
+      /* ??? There's no dwarf2 column reserved for SREG.  */
+      emit_move_insn (tmp_reg_rtx, gen_rtx_MEM (QImode, GEN_INT (SREG_ADDR)));
+      emit_push_byte (TMP_REGNO, false);
 
       /* Push RAMPZ.  */
-      if(AVR_HAVE_RAMPZ 
-         && (TEST_HARD_REG_BIT (set, REG_Z) && TEST_HARD_REG_BIT (set, REG_Z + 1)))
+      /* ??? There's no dwarf2 column reserved for RAMPZ.  */
+      if (AVR_HAVE_RAMPZ 
+          && TEST_HARD_REG_BIT (set, REG_Z)
+          && TEST_HARD_REG_BIT (set, REG_Z + 1))
         {
-          insn = emit_move_insn (tmp_reg_rtx, 
-                                 gen_rtx_MEM (QImode, GEN_INT (RAMPZ_ADDR)));
-          RTX_FRAME_RELATED_P (insn) = 1;
-          insn = emit_move_insn (pushbyte, tmp_reg_rtx);
-          RTX_FRAME_RELATED_P (insn) = 1;
-          cfun->machine->stack_usage++;
+          emit_move_insn (tmp_reg_rtx,
+			  gen_rtx_MEM (QImode, GEN_INT (RAMPZ_ADDR)));
+	  emit_push_byte (TMP_REGNO, false);
         }
 	
       /* Clear zero reg.  */
-      insn = emit_move_insn (zero_reg_rtx, const0_rtx);
-      RTX_FRAME_RELATED_P (insn) = 1;
+      emit_move_insn (zero_reg_rtx, const0_rtx);
 
       /* Prevent any attempt to delete the setting of ZERO_REG!  */
       emit_use (zero_reg_rtx);
@@ -623,37 +635,63 @@  expand_prologue (void)
 		   || (AVR_2_BYTE_PC && live_seq > 6)
 		   || live_seq > 7)) 
     {
-      insn = emit_move_insn (gen_rtx_REG (HImode, REG_X), 
-                             gen_int_mode (size, HImode));
-      RTX_FRAME_RELATED_P (insn) = 1;
+      int first_reg, reg, offset;
 
-      insn = 
-        emit_insn (gen_call_prologue_saves (gen_int_mode (live_seq, HImode),
-					    gen_int_mode (size + live_seq, HImode)));
+      emit_move_insn (gen_rtx_REG (HImode, REG_X), 
+                      gen_int_mode (size, HImode));
+
+      insn = emit_insn (gen_call_prologue_saves
+			(gen_int_mode (live_seq, HImode),
+		         gen_int_mode (size + live_seq, HImode)));
       RTX_FRAME_RELATED_P (insn) = 1;
+
+      /* Describe the effect of the unspec_volatile call to prologue_saves.
+	 Note that this formulation assumes that add_reg_note pushes the
+	 notes to the front.  Thus we build them in the reverse order of
+	 how we want dwarf2out to process them.  */
+
+      /* The function does always set frame_pointer_rtx, but whether that
+	 is going to be permanent in the function is frame_pointer_needed.  */
+      add_reg_note (insn, REG_CFA_ADJUST_CFA,
+		    gen_rtx_SET (VOIDmode,
+				 (frame_pointer_needed
+				  ? frame_pointer_rtx : stack_pointer_rtx),
+				 plus_constant (stack_pointer_rtx,
+						-(size + live_seq))));
+
+      /* Note that live_seq always contains r28+r29, but the other
+	 registers to be saved are all below 18.  */
+      first_reg = 18 - (live_seq - 2);
+
+      for (reg = 29, offset = -live_seq + 1;
+	   reg >= first_reg;
+	   reg = (reg == 28 ? 17 : reg - 1), ++offset)
+	{
+	  rtx m, r;
+
+	  m = gen_rtx_MEM (QImode, plus_constant (stack_pointer_rtx, offset));
+	  r = gen_rtx_REG (QImode, reg);
+	  add_reg_note (insn, REG_CFA_OFFSET, gen_rtx_SET (VOIDmode, m, r));
+	}
+
       cfun->machine->stack_usage += size + live_seq;
     }
   else
     {
       int reg;
       for (reg = 0; reg < 32; ++reg)
-        {
-          if (TEST_HARD_REG_BIT (set, reg))
-            {
-              /* Emit push of register to save.  */
-              insn=emit_move_insn (pushbyte, gen_rtx_REG (QImode, reg));
-              RTX_FRAME_RELATED_P (insn) = 1;
-              cfun->machine->stack_usage++;
-            }
-        }
+        if (TEST_HARD_REG_BIT (set, reg))
+	  emit_push_byte (reg, true);
+
       if (frame_pointer_needed)
         {
 	  if (!(cfun->machine->is_OS_task || cfun->machine->is_OS_main))
 	    {
-              /* Push frame pointer.  */
-	      insn = emit_move_insn (pushword, frame_pointer_rtx);
-              RTX_FRAME_RELATED_P (insn) = 1;
-	      cfun->machine->stack_usage += 2;
+              /* Push frame pointer.  Always be consistent about the
+		 ordering of pushes -- epilogue_restores expects the
+		 register pair to be pushed low byte first.  */
+	      emit_push_byte (REG_Y, true);
+	      emit_push_byte (REG_Y + 1, true);
 	    }
 
           if (!size)
@@ -676,13 +714,12 @@  expand_prologue (void)
               is selected.  */
               rtx myfp;
 	      rtx fp_plus_insns; 
-	      rtx sp_plus_insns = NULL_RTX;
 
               if (AVR_HAVE_8BIT_SP)
                 {
-                  /* The high byte (r29) doesn't change - prefer 'subi' (1 cycle)
-                     over 'sbiw' (2 cycles, same size).  */
-                  myfp = gen_rtx_REG (QImode, REGNO (frame_pointer_rtx));
+                  /* The high byte (r29) doesn't change.  Prefer 'subi'
+		     (1 cycle) over 'sbiw' (2 cycles, same size).  */
+                  myfp = gen_rtx_REG (QImode, FRAME_POINTER_REGNUM);
                 }
               else 
                 {
@@ -693,41 +730,43 @@  expand_prologue (void)
 	      /* Method 1-Adjust frame pointer.  */
 	      start_sequence ();
 
-              insn = emit_move_insn (frame_pointer_rtx, stack_pointer_rtx);
-              RTX_FRAME_RELATED_P (insn) = 1;
+	      /* Normally the dwarf2out frame-related-expr interpreter does
+		 not expect to have the CFA change once the frame pointer is
+		 set up.  Thus we avoid marking the move insn below and
+		 instead indicate that the entire operation is complete after
+		 the frame pointer subtraction is done.  */
 
-              insn = 
-	        emit_move_insn (myfp,
-				gen_rtx_PLUS (GET_MODE(myfp), myfp, 
-					      gen_int_mode (-size, 
-							    GET_MODE(myfp))));
-              RTX_FRAME_RELATED_P (insn) = 1;
+              emit_move_insn (frame_pointer_rtx, stack_pointer_rtx);
 
-	      /* Copy to stack pointer.  */
+              insn = emit_move_insn (myfp, plus_constant (myfp, -size));
+              RTX_FRAME_RELATED_P (insn) = 1;
+	      add_reg_note (insn, REG_CFA_ADJUST_CFA,
+			    gen_rtx_SET (VOIDmode, frame_pointer_rtx,
+					 plus_constant (stack_pointer_rtx,
+							-size)));
+
+	      /* Copy to stack pointer.  Note that since we've already
+		 changed the CFA to the frame pointer this operation
+		 need not be annotated at all.  */
 	      if (AVR_HAVE_8BIT_SP)
 		{
-		  insn = emit_move_insn (stack_pointer_rtx, frame_pointer_rtx);
-		  RTX_FRAME_RELATED_P (insn) = 1;
+		  emit_move_insn (stack_pointer_rtx, frame_pointer_rtx);
 		}
 	      else if (TARGET_NO_INTERRUPTS 
 		       || cfun->machine->is_signal
 		       || cfun->machine->is_OS_main)
 		{
-		  insn = 
-		    emit_insn (gen_movhi_sp_r_irq_off (stack_pointer_rtx, 
-						       frame_pointer_rtx));
-		  RTX_FRAME_RELATED_P (insn) = 1;		
+		  emit_insn (gen_movhi_sp_r_irq_off (stack_pointer_rtx, 
+						     frame_pointer_rtx));
 		}
 	      else if (cfun->machine->is_interrupt)
 		{
-		  insn = emit_insn (gen_movhi_sp_r_irq_on (stack_pointer_rtx, 
-							   frame_pointer_rtx));
-		  RTX_FRAME_RELATED_P (insn) = 1;
+		  emit_insn (gen_movhi_sp_r_irq_on (stack_pointer_rtx, 
+						    frame_pointer_rtx));
 		}
 	      else
 		{
-		  insn = emit_move_insn (stack_pointer_rtx, frame_pointer_rtx);
-		  RTX_FRAME_RELATED_P (insn) = 1;
+		  emit_move_insn (stack_pointer_rtx, frame_pointer_rtx);
 		}
 
 	      fp_plus_insns = get_insns ();
@@ -736,30 +775,30 @@  expand_prologue (void)
 	      /* Method 2-Adjust Stack pointer.  */
               if (size <= 6)
                 {
+		  rtx sp_plus_insns;
+
 		  start_sequence ();
 
-		  insn = 
-		    emit_move_insn (stack_pointer_rtx,
-				    gen_rtx_PLUS (HImode, 
-						  stack_pointer_rtx, 
-						  gen_int_mode (-size, 
-								HImode)));
+	          insn = plus_constant (stack_pointer_rtx, -size);
+		  insn = emit_move_insn (stack_pointer_rtx, insn);
 		  RTX_FRAME_RELATED_P (insn) = 1;
 		  
-		  insn = 
-		    emit_move_insn (frame_pointer_rtx, stack_pointer_rtx);
+		  insn = emit_move_insn (frame_pointer_rtx, stack_pointer_rtx);
 		  RTX_FRAME_RELATED_P (insn) = 1;
 
 		  sp_plus_insns = get_insns ();
 		  end_sequence ();
-                }
 
-              /* Use shortest method.  */
-              if (size <= 6 && (get_sequence_length (sp_plus_insns) 
-				 < get_sequence_length (fp_plus_insns)))
-		emit_insn (sp_plus_insns);
-              else
+		  /* Use shortest method.  */
+		  if (get_sequence_length (sp_plus_insns) 
+		      < get_sequence_length (fp_plus_insns))
+		    emit_insn (sp_plus_insns);
+		  else
+		    emit_insn (fp_plus_insns);
+                }
+	      else
 		emit_insn (fp_plus_insns);
+
 	      cfun->machine->stack_usage += size;
             }
         }
@@ -813,6 +852,20 @@  avr_epilogue_uses (int regno ATTRIBUTE_UNUSED)
   return 0;
 }
 
+/*  Helper for expand_epilogue.  Emit a pop of a byte register.  */
+
+static void
+emit_pop_byte (unsigned regno)
+{
+  rtx mem, reg;
+
+  mem = gen_rtx_PRE_INC (HImode, stack_pointer_rtx);
+  mem = gen_frame_mem (QImode, mem);
+  reg = gen_rtx_REG (QImode, regno);
+
+  emit_insn (gen_rtx_SET (VOIDmode, reg, mem));
+}
+
 /*  Output RTL epilogue.  */
 
 void
@@ -865,13 +918,12 @@  expand_epilogue (void)
               /* Try two methods to adjust stack and select shortest.  */
 	      rtx myfp;
 	      rtx fp_plus_insns;
-	      rtx sp_plus_insns = NULL_RTX;
-	      
+
 	      if (AVR_HAVE_8BIT_SP)
                 {
                   /* The high byte (r29) doesn't change - prefer 'subi' 
                      (1 cycle) over 'sbiw' (2 cycles, same size).  */
-                  myfp = gen_rtx_REG (QImode, REGNO (frame_pointer_rtx));
+                  myfp = gen_rtx_REG (QImode, FRAME_POINTER_REGNUM);
                 }
               else 
                 {
@@ -882,10 +934,7 @@  expand_epilogue (void)
               /* Method 1-Adjust frame pointer.  */
 	      start_sequence ();
 
-	      emit_move_insn (myfp,
-			      gen_rtx_PLUS (GET_MODE (myfp), myfp,
-					    gen_int_mode (size, 
-							  GET_MODE(myfp))));
+	      emit_move_insn (myfp, plus_constant (myfp, size));
 
 	      /* Copy to stack pointer.  */
 	      if (AVR_HAVE_8BIT_SP)
@@ -914,58 +963,63 @@  expand_epilogue (void)
               /* Method 2-Adjust Stack pointer.  */
               if (size <= 5)
                 {
+		  rtx sp_plus_insns;
+
 		  start_sequence ();
 
 		  emit_move_insn (stack_pointer_rtx,
-				  gen_rtx_PLUS (HImode, stack_pointer_rtx,
-						gen_int_mode (size, 
-							      HImode)));
+				  plus_constant (stack_pointer_rtx, size));
 
 		  sp_plus_insns = get_insns ();
 		  end_sequence ();
-                }
 
-              /* Use shortest method.  */
-              if (size <= 5 && (get_sequence_length (sp_plus_insns) 
-				 < get_sequence_length (fp_plus_insns)))
-	      	emit_insn (sp_plus_insns);
-              else
+		  /* Use shortest method.  */
+		  if (get_sequence_length (sp_plus_insns) 
+		      < get_sequence_length (fp_plus_insns))
+		    emit_insn (sp_plus_insns);
+		  else
+		    emit_insn (fp_plus_insns);
+                }
+	      else
 		emit_insn (fp_plus_insns);
             }
 	  if (!(cfun->machine->is_OS_task || cfun->machine->is_OS_main))
 	    {
-              /* Restore previous frame_pointer.  */
-	      emit_insn (gen_pophi (frame_pointer_rtx));
+              /* Restore previous frame_pointer.  See expand_prologue for
+		 rationale for not using pophi.  */
+	      emit_pop_byte (REG_Y + 1);
+	      emit_pop_byte (REG_Y);
 	    }
 	}
+
       /* Restore used registers.  */
       for (reg = 31; reg >= 0; --reg)
-        {
-          if (TEST_HARD_REG_BIT (set, reg))
-              emit_insn (gen_popqi (gen_rtx_REG (QImode, reg)));
-        }
+        if (TEST_HARD_REG_BIT (set, reg))
+          emit_pop_byte (reg);
+
       if (cfun->machine->is_interrupt || cfun->machine->is_signal)
         {
           /* Restore RAMPZ using tmp reg as scratch.  */
-	  if(AVR_HAVE_RAMPZ 
-             && (TEST_HARD_REG_BIT (set, REG_Z) && TEST_HARD_REG_BIT (set, REG_Z + 1)))
+	  if (AVR_HAVE_RAMPZ 
+              && TEST_HARD_REG_BIT (set, REG_Z)
+	      && TEST_HARD_REG_BIT (set, REG_Z + 1))
             {
-	      emit_insn (gen_popqi (tmp_reg_rtx));
-	      emit_move_insn (gen_rtx_MEM(QImode, GEN_INT(RAMPZ_ADDR)), 
+	      emit_pop_byte (TMP_REGNO);
+	      emit_move_insn (gen_rtx_MEM (QImode, GEN_INT (RAMPZ_ADDR)), 
 			      tmp_reg_rtx);
 	    }
 
           /* Restore SREG using tmp reg as scratch.  */
-          emit_insn (gen_popqi (tmp_reg_rtx));
+          emit_pop_byte (TMP_REGNO);
       
-          emit_move_insn (gen_rtx_MEM(QImode, GEN_INT(SREG_ADDR)), 
+          emit_move_insn (gen_rtx_MEM (QImode, GEN_INT (SREG_ADDR)), 
 			  tmp_reg_rtx);
 
           /* Restore tmp REG.  */
-          emit_insn (gen_popqi (tmp_reg_rtx));
+          emit_pop_byte (TMP_REGNO);
 
           /* Restore zero REG.  */
-          emit_insn (gen_popqi (zero_reg_rtx));
+          emit_pop_byte (ZERO_REGNO);
         }
 
       emit_jump_insn (gen_return ());
diff --git a/gcc/config/avr/avr.h b/gcc/config/avr/avr.h
index 9a71078..ee36daf 100644
--- a/gcc/config/avr/avr.h
+++ b/gcc/config/avr/avr.h
@@ -351,9 +351,6 @@  enum reg_class {
 
 #define STATIC_CHAIN_REGNUM 2
 
-/* Offset from the frame pointer register value to the top of the stack.  */
-#define FRAME_POINTER_CFA_OFFSET(FNDECL) 0
-
 #define ELIMINABLE_REGS {					\
       {ARG_POINTER_REGNUM, FRAME_POINTER_REGNUM},		\
 	{FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM}		\
@@ -809,6 +806,13 @@  mmcu=*:-mmcu=%*}"
 
 #define OBJECT_FORMAT_ELF
 
+#define INCOMING_RETURN_ADDR_RTX   avr_incoming_return_addr_rtx ()
+#define INCOMING_FRAME_SP_OFFSET   (AVR_3_BYTE_PC ? 3 : 2)
+
+/* The caller's stack pointer value immediately before the call
+   is one byte below the first argument.  */
+#define ARG_POINTER_CFA_OFFSET(FNDECL)  -1
+
 #define HARD_REGNO_RENAME_OK(OLD_REG, NEW_REG) \
   avr_hard_regno_rename_ok (OLD_REG, NEW_REG)
 
diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md
index f086e80..f4a95d8 100644
--- a/gcc/config/avr/avr.md
+++ b/gcc/config/avr/avr.md
@@ -182,7 +182,7 @@ 
 
 
 (define_insn "*pushqi"
-  [(set (mem:QI (post_dec (reg:HI REG_SP)))
+  [(set (mem:QI (post_dec:HI (reg:HI REG_SP)))
         (match_operand:QI 0 "reg_or_0_operand" "r,L"))]
   ""
   "@
@@ -190,9 +190,8 @@ 
 	push __zero_reg__"
   [(set_attr "length" "1,1")])
 
-
 (define_insn "*pushhi"
-  [(set (mem:HI (post_dec (reg:HI REG_SP)))
+  [(set (mem:HI (post_dec:HI (reg:HI REG_SP)))
         (match_operand:HI 0 "reg_or_0_operand" "r,L"))]
   ""
   "@
@@ -201,7 +200,7 @@ 
   [(set_attr "length" "2,2")])
 
 (define_insn "*pushsi"
-  [(set (mem:SI (post_dec (reg:HI REG_SP)))
+  [(set (mem:SI (post_dec:HI (reg:HI REG_SP)))
         (match_operand:SI 0 "reg_or_0_operand" "r,L"))]
   ""
   "@
@@ -210,7 +209,7 @@ 
   [(set_attr "length" "4,4")])
 
 (define_insn "*pushsf"
-  [(set (mem:SF (post_dec (reg:HI REG_SP)))
+  [(set (mem:SF (post_dec:HI (reg:HI REG_SP)))
         (match_operand:SF 0 "register_operand" "r"))]
   ""
   "push %D0
@@ -3127,20 +3126,12 @@ 
 
 (define_insn "popqi"
   [(set (match_operand:QI 0 "register_operand" "=r")
-        (mem:QI (post_inc (reg:HI REG_SP))))]
+        (mem:QI (pre_inc:HI (reg:HI REG_SP))))]
   ""
   "pop %0"
   [(set_attr "cc" "none")
    (set_attr "length" "1")])
 
-(define_insn "pophi"
-  [(set (match_operand:HI 0 "register_operand" "=r")
-        (mem:HI (post_inc (reg:HI REG_SP))))]
-  ""
-  "pop %A0\;pop %B0"
-  [(set_attr "cc" "none")
-   (set_attr "length" "2")])
-
 ;; Enable Interrupts
 (define_insn "enable_interrupt"
   [(unspec [(const_int 0)] UNSPEC_SEI)]
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index fea8209..b5e660c 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -2240,7 +2240,7 @@  dwarf2out_frame_debug_cfa_restore (rtx reg, const char *label)
 	   cfa.base_offset = -cfa_store.offset
 
   Rule 11:
-  (set (mem ({pre_inc,pre_dec} sp:cfa_store.reg)) <reg>)
+  (set (mem ({pre_inc,pre_dec,post_dec} sp:cfa_store.reg)) <reg>)
   effects: cfa_store.offset += -/+ mode_size(mem)
 	   cfa.offset = cfa_store.offset if cfa.reg == sp
 	   cfa.reg = sp
@@ -2259,7 +2259,7 @@  dwarf2out_frame_debug_cfa_restore (rtx reg, const char *label)
 	   cfa.base_offset = -{cfa_store,cfa_temp}.offset
 
   Rule 14:
-  (set (mem (postinc <reg1>:cfa_temp <const_int>)) <reg2>)
+  (set (mem (post_inc <reg1>:cfa_temp <const_int>)) <reg2>)
   effects: cfa.reg = <reg1>
 	   cfa.base_offset = -cfa_temp.offset
 	   cfa_temp.offset -= mode_size(mem)
@@ -2592,6 +2592,7 @@  dwarf2out_frame_debug_expr (rtx expr, const char *label)
 	  /* Rule 11 */
 	case PRE_INC:
 	case PRE_DEC:
+	case POST_DEC:
 	  offset = GET_MODE_SIZE (GET_MODE (dest));
 	  if (GET_CODE (XEXP (dest, 0)) == PRE_INC)
 	    offset = -offset;
@@ -2616,7 +2617,10 @@  dwarf2out_frame_debug_expr (rtx expr, const char *label)
 	  if (cfa.reg == STACK_POINTER_REGNUM)
 	    cfa.offset = cfa_store.offset;
 
-	  offset = -cfa_store.offset;
+	  if (GET_CODE (XEXP (dest, 0)) == POST_DEC)
+	    offset += -cfa_store.offset;
+	  else
+	    offset = -cfa_store.offset;
 	  break;
 
 	  /* Rule 12 */