diff mbox

[AVR] PR52461: Fix RAMPZ clobbering and RAMP* in epilogue

Message ID 4F53B053.8070302@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay March 4, 2012, 6:11 p.m. UTC 
This patch fixes several issues with RAMP registers:

* On Devices with more than 64 KiB RAM, RAMPZ is used as high-byte of
  RAM address. If RAMPZ is used to read flash, it must be reset to 0
  after the read so that RAM-read will operate correctly in the remainder.
  There is no support for RAM > 64 Ki so RAMPZ = 0 is in order.

* The ISR epilogue restored RAMP* registers in the wrong order.

* As RAMPZ is used both in ELPM and LD/LDD on some xmega core, the right
  condition to set RAMPZ prior to ELPM is "have ELPM", not "have RAMPZ".

* Never read unintentionally from RAM because a flash address interpreted
  as a RAM address might point to the I/O area.

Ok for trunk and 4.7?

Johann

libgcc/
	PR target/52461
	* config/avr/lib1funcs.S (__do_copy_data): Clear RAMPZ after usage
	if RAMPZ affects reading from RAM.
	(__tablejump_elpm__): Ditto.
	(.xload): Ditto.
	(__movmemx_hi): Ditto.
	(__do_global_ctors): Right condition for RAMPZ usage is "have ELPM".
	(__do_global_dtors): Ditto.
	(__xload_1, __xload_2, __xload_3, __xload_4): Ditto.  And make weak.
	(__movmemx_hi): Ditto.  And fix RAM-loop label.
	(__xload_1): Never read unintentionally from RAM.

gcc/
	PR target/52461
	* gcc/config/avr/avr.c (expand_prologue): Depend save/restore of
	RAMPZ on HAVE_RAMPD, not HAVE_RAMPZ.
	(expand_epilogue): Ditto.  And fix order of restoration to:
	RAMPZ, RAMPY, RAMPX, RAMPD.
	(avr_xload_libgcc_p): Always load __memx by lilbgcc call on
	big-RAM devices.
	(avr_out_lpm): Clear RAMPZ after usage if RAMPZ affects reading
	from RAM.
	(avr_out_xload): Never read unintentionally from RAM.
	* config/avr/avr.md (xload_8): Adjust insn length.

Comments

Georg-Johann Lay March 5, 2012, 2:11 p.m. UTC | #1 
Georg-Johann Lay wrote:
> This patch fixes several issues with RAMP registers:
> 
> * On Devices with more than 64 KiB RAM, RAMPZ is used as high-byte of
>   RAM address. If RAMPZ is used to read flash, it must be reset to 0
>   after the read so that RAM-read will operate correctly in the remainder.
>   There is no support for RAM > 64 Ki so RAMPZ = 0 is in order.
> 
> * The ISR epilogue restored RAMP* registers in the wrong order.
> 
> * As RAMPZ is used both in ELPM and LD/LDD on some xmega core, the right
>   condition to set RAMPZ prior to ELPM is "have ELPM", not "have RAMPZ".
> 
> * Never read unintentionally from RAM because a flash address interpreted
>   as a RAM address might point to the I/O area.
> 
> Ok for trunk and 4.7?

The following change in expand_prologue is wrong:

-      if (AVR_HAVE_RAMPZ
+      if (AVR_HAVE_RAMPD /* sic! */

similar in expand_epilogue.

Ok without that change?

> 
> Johann
> 
> libgcc/
> 	PR target/52461
> 	* config/avr/lib1funcs.S (__do_copy_data): Clear RAMPZ after usage
> 	if RAMPZ affects reading from RAM.
> 	(__tablejump_elpm__): Ditto.
> 	(.xload): Ditto.
> 	(__movmemx_hi): Ditto.
> 	(__do_global_ctors): Right condition for RAMPZ usage is "have ELPM".
> 	(__do_global_dtors): Ditto.
> 	(__xload_1, __xload_2, __xload_3, __xload_4): Ditto.  And make weak.
> 	(__movmemx_hi): Ditto.  And fix RAM-loop label.
> 	(__xload_1): Never read unintentionally from RAM.
> 
> gcc/
> 	PR target/52461
> 	* gcc/config/avr/avr.c (expand_prologue): Depend save/restore of
> 	RAMPZ on HAVE_RAMPD, not HAVE_RAMPZ.
> 	(expand_epilogue): Ditto.  And fix order of restoration to:
> 	RAMPZ, RAMPY, RAMPX, RAMPD.
> 	(avr_xload_libgcc_p): Always load __memx by lilbgcc call on
> 	big-RAM devices.
> 	(avr_out_lpm): Clear RAMPZ after usage if RAMPZ affects reading
> 	from RAM.
> 	(avr_out_xload): Never read unintentionally from RAM.
> 	* config/avr/avr.md (xload_8): Adjust insn length.
Richard Biener March 5, 2012, 2:26 p.m. UTC | #2 
On Mon, Mar 5, 2012 at 3:11 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> Georg-Johann Lay wrote:
>> This patch fixes several issues with RAMP registers:
>>
>> * On Devices with more than 64 KiB RAM, RAMPZ is used as high-byte of
>>   RAM address. If RAMPZ is used to read flash, it must be reset to 0
>>   after the read so that RAM-read will operate correctly in the remainder.
>>   There is no support for RAM > 64 Ki so RAMPZ = 0 is in order.
>>
>> * The ISR epilogue restored RAMP* registers in the wrong order.
>>
>> * As RAMPZ is used both in ELPM and LD/LDD on some xmega core, the right
>>   condition to set RAMPZ prior to ELPM is "have ELPM", not "have RAMPZ".
>>
>> * Never read unintentionally from RAM because a flash address interpreted
>>   as a RAM address might point to the I/O area.
>>
>> Ok for trunk and 4.7?

All commits to the 4.7 branch need explicit release manager approval.  AVR
isn't primary/secondary so please do not change anything before is
released 4.7.0 for it.

Thanks,
Richard.

> The following change in expand_prologue is wrong:
>
> -      if (AVR_HAVE_RAMPZ
> +      if (AVR_HAVE_RAMPD /* sic! */
>
> similar in expand_epilogue.
>
> Ok without that change?
>
>>
>> Johann
>>
>> libgcc/
>>       PR target/52461
>>       * config/avr/lib1funcs.S (__do_copy_data): Clear RAMPZ after usage
>>       if RAMPZ affects reading from RAM.
>>       (__tablejump_elpm__): Ditto.
>>       (.xload): Ditto.
>>       (__movmemx_hi): Ditto.
>>       (__do_global_ctors): Right condition for RAMPZ usage is "have ELPM".
>>       (__do_global_dtors): Ditto.
>>       (__xload_1, __xload_2, __xload_3, __xload_4): Ditto.  And make weak.
>>       (__movmemx_hi): Ditto.  And fix RAM-loop label.
>>       (__xload_1): Never read unintentionally from RAM.
>>
>> gcc/
>>       PR target/52461
>>       * gcc/config/avr/avr.c (expand_prologue): Depend save/restore of
>>       RAMPZ on HAVE_RAMPD, not HAVE_RAMPZ.
>>       (expand_epilogue): Ditto.  And fix order of restoration to:
>>       RAMPZ, RAMPY, RAMPX, RAMPD.
>>       (avr_xload_libgcc_p): Always load __memx by lilbgcc call on
>>       big-RAM devices.
>>       (avr_out_lpm): Clear RAMPZ after usage if RAMPZ affects reading
>>       from RAM.
>>       (avr_out_xload): Never read unintentionally from RAM.
>>       * config/avr/avr.md (xload_8): Adjust insn length.
Georg-Johann Lay March 5, 2012, 3:25 p.m. UTC | #3 
Richard Guenther wrote:

> All commits to the 4.7 branch need explicit release manager approval.  AVR
> isn't primary/secondary so please do not change anything before is
> released 4.7.0 for it.
> 
> Thanks,
> Richard.

What is the exact procedure in that case?
Wait until approve from release manager in that case?
Who is the release manager, and should I CC for such changes?
Or just hope the patch is not overseen.

Johann
Richard Biener March 5, 2012, 3:30 p.m. UTC | #4 
On Mon, Mar 5, 2012 at 4:25 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> Richard Guenther wrote:
>
>> All commits to the 4.7 branch need explicit release manager approval.  AVR
>> isn't primary/secondary so please do not change anything before is
>> released 4.7.0 for it.
>>
>> Thanks,
>> Richard.
>
> What is the exact procedure in that case?
> Wait until approve from release manager in that case?
> Who is the release manager, and should I CC for such changes?
> Or just hope the patch is not overseen.

The exact procedure is to do bugfixing during stage3/4, for release blockers
that pop up after a release candidate is created (like now), CC a release
manager (Jakub, me, Joseph) for patches that you like to get in even
though the branch is frozen.  Usually only bugs that prevent basic functionality
(like building a target) can be fixed at this point, for everything
else you have
to wait until after 4.7.0 is released and the branch opens again for regression
fixes.

Richard.

> Johann
>
diff mbox

Patch

Index: libgcc/config/avr/lib1funcs.S
===================================================================
--- libgcc/config/avr/lib1funcs.S	(revision 184887)
+++ libgcc/config/avr/lib1funcs.S	(working copy)
@@ -1853,7 +1853,7 @@  DEFUN __do_copy_data
 	cpi	r26, lo8(__data_end)
 	cpc	r27, r17
 	brne	.L__do_copy_data_loop
-#elif  !defined(__AVR_HAVE_ELPMX__) && defined(__AVR_HAVE_ELPM__)
+#elif defined(__AVR_HAVE_ELPM__)
 	ldi	r17, hi8(__data_end)
 	ldi	r26, lo8(__data_start)
 	ldi	r27, hi8(__data_start)
@@ -1873,7 +1873,7 @@  DEFUN __do_copy_data
 	cpi	r26, lo8(__data_end)
 	cpc	r27, r17
 	brne	.L__do_copy_data_loop
-#elif !defined(__AVR_HAVE_ELPMX__) && !defined(__AVR_HAVE_ELPM__)
+#else /* !ELPM */
 	ldi	r17, hi8(__data_end)
 	ldi	r26, lo8(__data_start)
 	ldi	r27, hi8(__data_start)
@@ -1892,7 +1892,11 @@  DEFUN __do_copy_data
 	cpi	r26, lo8(__data_end)
 	cpc	r27, r17
 	brne	.L__do_copy_data_loop
-#endif /* !defined(__AVR_HAVE_ELPMX__) && !defined(__AVR_HAVE_ELPM__) */
+#endif /* ELPMX / ELPM / LPM */
+#if defined (__AVR_HAVE_ELPM__) && defined (__AVR_HAVE_RAMPD__)
+	;; Reset RAMPZ to 0 so that EBI devices don't read garbage from RAM
+	out	__RAMPZ__, __zero_reg__
+#endif /* ELPM && RAMPD */
 ENDF __do_copy_data
 #endif /* L_copy_data */
 
@@ -1920,7 +1924,7 @@  ENDF __do_clear_bss
 #ifdef L_ctors
 	.section .init6,"ax",@progbits
 DEFUN __do_global_ctors
-#if defined(__AVR_HAVE_RAMPZ__)
+#if defined(__AVR_HAVE_ELPM__)
 	ldi	r17, hi8(__ctors_start)
 	ldi	r28, lo8(__ctors_end)
 	ldi	r29, hi8(__ctors_end)
@@ -1939,7 +1943,7 @@  DEFUN __do_global_ctors
 	ldi	r24, hh8(__ctors_start)
 	cpc	r16, r24
 	brne	.L__do_global_ctors_loop
-#else
+#else /* !ELPM */
 	ldi	r17, hi8(__ctors_start)
 	ldi	r28, lo8(__ctors_end)
 	ldi	r29, hi8(__ctors_end)
@@ -1953,14 +1957,14 @@  DEFUN __do_global_ctors
 	cpi	r28, lo8(__ctors_start)
 	cpc	r29, r17
 	brne	.L__do_global_ctors_loop
-#endif /* defined(__AVR_HAVE_RAMPZ__) */
+#endif /* defined(__AVR_HAVE_ELPM__) */
 ENDF __do_global_ctors
 #endif /* L_ctors */
 
 #ifdef L_dtors
 	.section .fini6,"ax",@progbits
 DEFUN __do_global_dtors
-#if defined(__AVR_HAVE_RAMPZ__)
+#if defined(__AVR_HAVE_ELPM__)
 	ldi	r17, hi8(__dtors_end)
 	ldi	r28, lo8(__dtors_start)
 	ldi	r29, hi8(__dtors_start)
@@ -1979,7 +1983,7 @@  DEFUN __do_global_dtors
 	ldi	r24, hh8(__dtors_end)
 	cpc	r16, r24
 	brne	.L__do_global_dtors_loop
-#else
+#else /* !ELPM */
 	ldi	r17, hi8(__dtors_end)
 	ldi	r28, lo8(__dtors_start)
 	ldi	r29, hi8(__dtors_start)
@@ -1993,7 +1997,7 @@  DEFUN __do_global_dtors
 	cpi	r28, lo8(__dtors_end)
 	cpc	r29, r17
 	brne	.L__do_global_dtors_loop
-#endif /* defined(__AVR_HAVE_RAMPZ__) */
+#endif /* defined(__AVR_HAVE_ELPM__) */
 ENDF __do_global_dtors
 #endif /* L_dtors */
 
@@ -2001,18 +2005,21 @@  ENDF __do_global_dtors
     
 #ifdef L_tablejump_elpm
 DEFUN __tablejump_elpm__
-#if defined (__AVR_HAVE_ELPM__)
-#if defined (__AVR_HAVE_LPMX__)
+#if defined (__AVR_HAVE_ELPMX__)
 	elpm	__tmp_reg__, Z+
 	elpm	r31, Z
 	mov	r30, __tmp_reg__
+#if defined (__AVR_HAVE_RAMPD__)
+	;; Reset RAMPZ to 0 so that EBI devices don't read garbage from RAM
+	out	__RAMPZ__, __zero_reg__
+#endif /* RAMPD */
 #if defined (__AVR_HAVE_EIJMP_EICALL__)
 	eijmp
 #else
 	ijmp
-#endif
+#endif /* EICALL */
 
-#else
+#elif defined (__AVR_HAVE_ELPM__)
 	elpm
 	adiw	r30, 1
 	push	r0
@@ -2021,10 +2028,9 @@  DEFUN __tablejump_elpm__
 #if defined (__AVR_HAVE_EIJMP_EICALL__)
 	in      __tmp_reg__, __EIND__
 	push    __tmp_reg__
-#endif
+#endif /* EICALL */
 	ret
-#endif
-#endif /* defined (__AVR_HAVE_ELPM__) */
+#endif /* ELPM */
 ENDF __tablejump_elpm__
 #endif /* defined (L_tablejump_elpm) */
 
@@ -2114,11 +2120,19 @@  ENDF __load_4
     adiw    r30, 1
 .endif
 #endif
+#if defined (__AVR_HAVE_ELPM__) && defined (__AVR_HAVE_RAMPD__)
+.if \dest == D0+\n-1
+	;; Reset RAMPZ to 0 so that EBI devices don't read garbage from RAM
+    out     __RAMPZ__, __zero_reg__
+.endif
+#endif
 .endm ; .xload
 
 #if defined (L_xload_1)
 DEFUN __xload_1
-#if defined (__AVR_HAVE_LPMX__) && !defined (__AVR_HAVE_RAMPZ__)
+.weak __xload_1
+#if defined (__AVR_HAVE_LPMX__) && !defined (__AVR_HAVE_ELPM__)
+    sbrc    HHI8, 7
     ld      D0, Z
     sbrs    HHI8, 7
     lpm     D0, Z
@@ -2126,24 +2140,25 @@  DEFUN __xload_1
 #else
     sbrc    HHI8, 7
     rjmp    1f
-#if defined (__AVR_HAVE_RAMPZ__)
+#if defined (__AVR_HAVE_ELPM__)
     out     __RAMPZ__, HHI8
-#endif /* __AVR_HAVE_RAMPZ__ */
+#endif /* __AVR_HAVE_ELPM__ */
     .xload  D0, 1
     ret
 1:  ld      D0, Z
     ret
-#endif /* LPMx && ! RAMPZ */
+#endif /* LPMx && ! ELPM */
 ENDF __xload_1
 #endif /* L_xload_1 */
 
 #if defined (L_xload_2)
 DEFUN __xload_2
+.weak __xload_2
     sbrc    HHI8, 7
     rjmp    1f
-#if defined (__AVR_HAVE_RAMPZ__)
+#if defined (__AVR_HAVE_ELPM__)
     out     __RAMPZ__, HHI8
-#endif /* __AVR_HAVE_RAMPZ__ */
+#endif /* __AVR_HAVE_ELPM__ */
     .xload  D0, 2
     .xload  D1, 2
     ret
@@ -2155,11 +2170,12 @@  ENDF __xload_2
 
 #if defined (L_xload_3)
 DEFUN __xload_3
+.weak __xload_3
     sbrc    HHI8, 7
     rjmp    1f
-#if defined (__AVR_HAVE_RAMPZ__)
+#if defined (__AVR_HAVE_ELPM__)
     out     __RAMPZ__, HHI8
-#endif /* __AVR_HAVE_RAMPZ__ */
+#endif /* __AVR_HAVE_ELPM__ */
     .xload  D0, 3
     .xload  D1, 3
     .xload  D2, 3
@@ -2173,11 +2189,12 @@  ENDF __xload_3
 
 #if defined (L_xload_4)
 DEFUN __xload_4
+.weak __xload_4
     sbrc    HHI8, 7
     rjmp    1f
-#if defined (__AVR_HAVE_RAMPZ__)
+#if defined (__AVR_HAVE_ELPM__)
     out     __RAMPZ__, HHI8
-#endif /* __AVR_HAVE_RAMPZ__ */
+#endif /* __AVR_HAVE_ELPM__ */
     .xload  D0, 4
     .xload  D1, 4
     .xload  D2, 4
@@ -2205,6 +2222,7 @@  ENDF __xload_4
 #define LOOP  24
 
 DEFUN __movmemx_qi
+.weak __movmemx_qi
     ;; #Bytes to copy fity in 8 Bits (1..255)
     ;; Zero-extend Loop Counter
     clr     LOOP+1
@@ -2212,6 +2230,7 @@  DEFUN __movmemx_qi
 ENDF __movmemx_qi
 
 DEFUN __movmemx_hi
+.weak __movmemx_hi
 
 ;; Read from where?
     sbrc    HHI8, 7
@@ -2219,7 +2238,7 @@  DEFUN __movmemx_hi
 
 ;; Read from Flash
 
-#if defined (__AVR_HAVE_RAMPZ__)
+#if defined (__AVR_HAVE_ELPM__)
     out     __RAMPZ__, HHI8
 #endif
 
@@ -2243,6 +2262,10 @@  DEFUN __movmemx_hi
     st      X+, r0
     sbiw    LOOP, 1
     brne    0b
+#if defined (__AVR_HAVE_ELPM__) && defined (__AVR_HAVE_RAMPD__)
+	;; Reset RAMPZ to 0 so that EBI devices don't read garbage from RAM
+	out	__RAMPZ__, __zero_reg__
+#endif /* ELPM && RAMPD */
     ret
 
 ;; Read from RAM
@@ -2252,7 +2275,7 @@  DEFUN __movmemx_hi
     ;; and store that Byte to RAM Destination
     st      X+, r0
     sbiw    LOOP, 1
-    brne    0b
+    brne    1b
     ret
 ENDF __movmemx_hi
 
Index: gcc/config/avr/avr.md
===================================================================
--- gcc/config/avr/avr.md	(revision 184887)
+++ gcc/config/avr/avr.md	(working copy)
@@ -363,6 +363,11 @@  (define_split
 ;;========================================================================
 ;; Move stuff around
 
+;; "loadqi_libgcc"
+;; "loadhi_libgcc"
+;; "loadpsi_libgcc"    
+;; "loadsi_libgcc"    
+;; "loadsf_libgcc"    
 (define_expand "load<mode>_libgcc"
   [(set (match_dup 3)
         (match_dup 2))
@@ -377,7 +382,12 @@  (define_expand "load<mode>_libgcc"
     operands[1] = replace_equiv_address (operands[1], operands[3]);
     set_mem_addr_space (operands[1], ADDR_SPACE_FLASH);
   })
-    
+
+;; "load_qi_libgcc"
+;; "load_hi_libgcc"
+;; "load_psi_libgcc"    
+;; "load_si_libgcc"    
+;; "load_sf_libgcc"    
 (define_insn "load_<mode>_libgcc"
   [(set (reg:MOVMODE 22)
         (match_operand:MOVMODE 0 "memory_operand" "m,m"))]
@@ -418,6 +428,11 @@  (define_insn_and_split "xload8_A"
     DONE;
   })
 
+;; "xloadqi_A"
+;; "xloadhi_A"
+;; "xloadpsi_A"
+;; "xloadsi_A"
+;; "xloadsf_A"
 (define_insn_and_split "xload<mode>_A"
   [(set (match_operand:MOVMODE 0 "register_operand" "=r")
         (match_operand:MOVMODE 1 "memory_operand"    "m"))
@@ -461,7 +476,7 @@  (define_insn "xload_8"
   {
     return avr_out_xload (insn, operands, NULL);
   }
-  [(set_attr "length" "3,4")
+  [(set_attr "length" "4,4")
    (set_attr "adjust_len" "*,xload")
    (set_attr "isa" "lpmx,lpm")
    (set_attr "cc" "none")])
Index: gcc/config/avr/avr.c
===================================================================
--- gcc/config/avr/avr.c	(revision 184887)
+++ gcc/config/avr/avr.c	(working copy)
@@ -1123,7 +1123,7 @@  expand_prologue (void)
           emit_push_sfr (rampy_rtx, false /* frame-related */, true /* clr */);
         }
 
-      if (AVR_HAVE_RAMPZ 
+      if (AVR_HAVE_RAMPD /* sic! */
           && TEST_HARD_REG_BIT (set, REG_Z)
           && TEST_HARD_REG_BIT (set, REG_Z + 1))
         {
@@ -1347,12 +1347,12 @@  expand_epilogue (bool sibcall_p)
       /* Restore RAMPZ/Y/X/D using tmp_reg as scratch.
          The conditions to restore them must be tha same as in prologue.  */
       
-      if (AVR_HAVE_RAMPX
-          && TEST_HARD_REG_BIT (set, REG_X)
-          && TEST_HARD_REG_BIT (set, REG_X + 1))
+      if (AVR_HAVE_RAMPD /* sic! */
+          && TEST_HARD_REG_BIT (set, REG_Z)
+          && TEST_HARD_REG_BIT (set, REG_Z + 1))
         {
           emit_pop_byte (TMP_REGNO);
-          emit_move_insn (rampx_rtx, tmp_reg_rtx);
+          emit_move_insn (rampz_rtx, tmp_reg_rtx);
         }
 
       if (AVR_HAVE_RAMPY
@@ -1364,12 +1364,12 @@  expand_epilogue (bool sibcall_p)
           emit_move_insn (rampy_rtx, tmp_reg_rtx);
         }
 
-      if (AVR_HAVE_RAMPZ
-          && TEST_HARD_REG_BIT (set, REG_Z)
-          && TEST_HARD_REG_BIT (set, REG_Z + 1))
+      if (AVR_HAVE_RAMPX
+          && TEST_HARD_REG_BIT (set, REG_X)
+          && TEST_HARD_REG_BIT (set, REG_X + 1))
         {
           emit_pop_byte (TMP_REGNO);
-          emit_move_insn (rampz_rtx, tmp_reg_rtx);
+          emit_move_insn (rampx_rtx, tmp_reg_rtx);
         }
 
       if (AVR_HAVE_RAMPD)
@@ -2446,7 +2446,8 @@  avr_xload_libgcc_p (enum machine_mode mo
   int n_bytes = GET_MODE_SIZE (mode);
   
   return (n_bytes > 1
-          || avr_current_device->n_flash > 1);
+          || avr_current_device->n_flash > 1
+          || AVR_HAVE_RAMPD);
 }
 
 
@@ -2762,7 +2763,14 @@  avr_out_lpm (rtx insn, rtx *op, int *ple
       break; /* POST_INC */
 
     } /* switch CODE (addr) */
+
+  if (xop[4] == xstring_e && AVR_HAVE_RAMPD)
+    {
+      /* Reset RAMPZ to 0 so that EBI devices don't read garbage from RAM */
       
+      avr_asm_len ("out __RAMPZ__,__zero_reg__", xop, plen, 1);
+    }
+
   return "";
 }
 
@@ -2782,8 +2790,9 @@  avr_out_xload (rtx insn ATTRIBUTE_UNUSED
   if (plen)
     *plen = 0;
 
-  avr_asm_len ("ld %3,%a2" CR_TAB
-               "sbrs %1,7", xop, plen, 2);
+  avr_asm_len ("sbrc %1,7" CR_TAB
+               "ld %3,%a2" CR_TAB
+               "sbrs %1,7", xop, plen, 3);
 
   avr_asm_len (AVR_HAVE_LPMX ? "lpm %3,%a2" : "lpm", xop, plen, 1);