This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[Patch.AVR] Fix PR51002 (gcc part)
- From: Georg-Johann Lay <avr at gjlay dot de>
- To: gcc-patches at gcc dot gnu dot org
- Cc: Denis Chertykov <chertykov at gmail dot com>, Wim Lewis <wiml at hhhh dot org>, Eric Weddington <eric dot weddington at atmel dot com>, Joerg Wunsch <joerg_wunsch at uriah dot heep dot sax dot de>
- Date: Tue, 29 Nov 2011 17:09:04 +0100
- Subject: [Patch.AVR] Fix PR51002 (gcc part)
- References: <4ED4BEDE.4000600@gjlay.de>
Georg-Johann Lay wrote:
> For devices with 8-bit SP reading the high byte SP_H of SP will get garbage.
>
> The patch uses CLR instead of IN SP_H to "read" the high part of SP.
>
> There are two issues with this patch:
>
> == 1 ==
>
> I cannot really test it because for devices that small running test suite
> does not give usable results. So I just looked at the patch and the
> small test case like the following compiled
>
> $ avr-gcc-4.6.2 -Os -mmcu=attiny26 -m[no]call-prologues
>
> long long a, b;
>
> long long __attribute__((noinline,noclione))
> bar (char volatile *c)
> {
> *c = 1;
> return a+b;
> }
>
> long long foo()
> {
> char buf[16];
> return bar (buf);
> }
>
>
> int main (void)
> {
> return foo();
> }
>
>
> The C parts look fine but...
>
>
> == 2 ==
>
> The libgcc parts will still read garbage to R29 as explained in the
> FIXMEs there.
>
> Solving the FIXMEs can only be achieved by splitting multilibs avr2 and avr25,
> i.e. the mutlilibs that mix devices with/without SP.H, into avr2, avr21, avr24,
> avr25, say.
>
> I don't think it's a good idea to have real 8-bit SP/FP and that it would cause
> all sorts of trouble.
>
> Ok to commit to 4.6?
>
> What about splitting multilibs? Is this appropriate for 4.7?
>
> Johann
>
> PR target/51002
> * config/avr/libgcc.S (__prologue_saves__, __epilogue_restores__):
> Enclose parts using __SP_H__ in defined (__AVR_HAVE_8BIT_SP__).
> Add FIXME comments.
> * config/avr/avr.md (movhi_sp_r_irq_off, movhi_sp_r_irq_on): Set
> insn condition to !AVR_HAVE_8BIT_SP.
> * config/avr/avr.c (output_movhi): "clr%B0" instead of "in
> %B0,__SP_H__" if AVR_HAVE_8BIT_SP.
> (avr_file_start): Only print "__SP_H__ = 0x3e" if !AVR_HAVE_8BIT_SP.
>
This is similar patch for trunk:
libgcc/
PR target/51345
PR target/51002
* config/avr/lib1funcs.S (__prologue_saves__,
__epilogue_restores__, __divdi3_moddi3): Enclose parts using
__SP_H__ in defined (__AVR_HAVE_8BIT_SP__). Add FIXME comments.
gcc/
PR target/51002
* config/avr/avr.md (movhi_sp_r): Set insn condition to
!AVR_HAVE_8BIT_SP.
* config/avr/avr.c (output_movhi): Use "clr%B0" instead of "in
%B0,__SP_H__" if AVR_HAVE_8BIT_SP.
(avr_file_start): Only print "__SP_H__ = 0x3e" if !AVR_HAVE_8BIT_SP.
Index: gcc/config/avr/avr.md
===================================================================
--- gcc/config/avr/avr.md (revision 181773)
+++ gcc/config/avr/avr.md (working copy)
@@ -620,7 +620,7 @@ (define_insn "movhi_sp_r"
(unspec_volatile:HI [(match_operand:HI 1 "register_operand" "r,r")
(match_operand:HI 2 "const_int_operand" "L,P")]
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"
Index: gcc/config/avr/avr.c
===================================================================
--- gcc/config/avr/avr.c (revision 181773)
+++ gcc/config/avr/avr.c (working copy)
@@ -2872,78 +2872,77 @@ output_movqi (rtx insn, rtx operands[],
const char *
-output_movhi (rtx insn, rtx operands[], int *l)
+output_movhi (rtx insn, rtx xop[], int *plen)
{
- int dummy;
- rtx dest = operands[0];
- rtx src = operands[1];
- int *real_l = l;
+ rtx dest = xop[0];
+ rtx src = xop[1];
+
+ gcc_assert (GET_MODE_SIZE (GET_MODE (dest)) == 2);
if (avr_mem_pgm_p (src)
|| avr_mem_pgm_p (dest))
{
- return avr_out_lpm (insn, operands, real_l);
+ return avr_out_lpm (insn, xop, plen);
}
- if (!l)
- l = &dummy;
-
- if (register_operand (dest, HImode))
+ if (REG_P (dest))
{
- if (register_operand (src, HImode)) /* mov r,r */
- {
- if (test_hard_reg_class (STACK_REG, dest))
- {
- if (AVR_HAVE_8BIT_SP)
- return *l = 1, AS2 (out,__SP_L__,%A1);
- /* Use simple load of stack pointer if no interrupts are
- used. */
- else if (TARGET_NO_INTERRUPTS)
- return *l = 2, (AS2 (out,__SP_H__,%B1) CR_TAB
- AS2 (out,__SP_L__,%A1));
- *l = 5;
- return (AS2 (in,__tmp_reg__,__SREG__) CR_TAB
- "cli" CR_TAB
- AS2 (out,__SP_H__,%B1) CR_TAB
- AS2 (out,__SREG__,__tmp_reg__) CR_TAB
- AS2 (out,__SP_L__,%A1));
- }
- else if (test_hard_reg_class (STACK_REG, src))
- {
- *l = 2;
- return (AS2 (in,%A0,__SP_L__) CR_TAB
- AS2 (in,%B0,__SP_H__));
- }
+ if (REG_P (src)) /* mov r,r */
+ {
+ if (test_hard_reg_class (STACK_REG, dest))
+ {
+ if (AVR_HAVE_8BIT_SP)
+ return avr_asm_len ("out __SP_L__,%A1", xop, plen, -1);
+
+ /* Use simple load of SP if no interrupts are used. */
+
+ return TARGET_NO_INTERRUPTS
+ ? avr_asm_len ("out __SP_H__,%B1" CR_TAB
+ "out __SP_L__,%A1", xop, plen, -2)
+
+ : avr_asm_len ("in __tmp_reg__,__SREG__" CR_TAB
+ "cli" CR_TAB
+ "out __SP_H__,%B1" CR_TAB
+ "out __SREG__,__tmp_reg__" CR_TAB
+ "out __SP_L__,%A1", xop, plen, -5);
+ }
+ else if (test_hard_reg_class (STACK_REG, src))
+ {
+ return AVR_HAVE_8BIT_SP
+ ? avr_asm_len ("in %A0,__SP_L__" CR_TAB
+ "clr %B0", xop, plen, -2)
+
+ : avr_asm_len ("in %A0,__SP_L__" CR_TAB
+ "in %B0,__SP_H__", xop, plen, -2);
+ }
- if (AVR_HAVE_MOVW)
- {
- *l = 1;
- return (AS2 (movw,%0,%1));
- }
- else
- {
- *l = 2;
- return (AS2 (mov,%A0,%A1) CR_TAB
- AS2 (mov,%B0,%B1));
- }
- }
+ return AVR_HAVE_MOVW
+ ? avr_asm_len ("movw %0,%1", xop, plen, -1)
+
+ : avr_asm_len ("mov %A0,%A1" CR_TAB
+ "mov %B0,%B1", xop, plen, -2);
+ } /* REG_P (src) */
else if (CONSTANT_P (src))
{
- return output_reload_inhi (operands, NULL, real_l);
+ return output_reload_inhi (xop, NULL, plen);
+ }
+ else if (MEM_P (src))
+ {
+ return out_movhi_r_mr (insn, xop, plen); /* mov r,m */
}
- else if (GET_CODE (src) == MEM)
- return out_movhi_r_mr (insn, operands, real_l); /* mov r,m */
}
- else if (GET_CODE (dest) == MEM)
+ else if (MEM_P (dest))
{
rtx xop[2];
xop[0] = dest;
xop[1] = src == const0_rtx ? zero_reg_rtx : src;
- return out_movhi_mr_r (insn, xop, real_l);
+ return out_movhi_mr_r (insn, xop, plen);
}
+
fatal_insn ("invalid insn:", insn);
+
return "";
}
@@ -7272,16 +7271,19 @@ avr_file_start (void)
default_file_start ();
+ if (!AVR_HAVE_8BIT_SP)
+ fprintf (asm_out_file,
+ "__SP_H__ = 0x%02x\n"
+ -sfr_offset + SP_ADDR + 1);
+
fprintf (asm_out_file,
- "__SREG__ = 0x%02x\n"
- "__SP_H__ = 0x%02x\n"
"__SP_L__ = 0x%02x\n"
+ "__SREG__ = 0x%02x\n"
"__RAMPZ__ = 0x%02x\n"
"__tmp_reg__ = %d\n"
"__zero_reg__ = %d\n",
- -sfr_offset + SREG_ADDR,
- -sfr_offset + SP_ADDR + 1,
-sfr_offset + SP_ADDR,
+ -sfr_offset + SREG_ADDR,
-sfr_offset + RAMPZ_ADDR,
TMP_REGNO,
ZERO_REGNO);
Index: libgcc/config/avr/lib1funcs.S
===================================================================
--- libgcc/config/avr/lib1funcs.S (revision 181700)
+++ libgcc/config/avr/lib1funcs.S (working copy)
@@ -1149,7 +1149,14 @@ DEFUN __divdi3_moddi3
4: ;; Epilogue: Restore the Z = 12 Registers and return
in r28, __SP_L__
+#if defined (__AVR_HAVE_8BIT_SP__)
+;; FIXME: __AVR_HAVE_8BIT_SP__ is set on device level, not on core level
+;; so this lines are dead code. To make it work, devices without
+;; SP_H must get their own multilib(s).
+ clr r29
+#else
in r29, __SP_H__
+#endif /* #SP = 8/16 */
ldi r30, 12
XJMP __epilogue_restores__ + ((18 - 12) * 2)
@@ -1229,6 +1236,15 @@ DEFUN __prologue_saves__
push r17
push r28
push r29
+#if defined (__AVR_HAVE_8BIT_SP__)
+;; FIXME: __AVR_HAVE_8BIT_SP__ is set on device level, not on core level
+;; so this lines are dead code. To make it work, devices without
+;; SP_H must get their own multilib(s).
+ in r28,__SP_L__
+ sub r28,r26
+ out __SP_L__,r28
+ clr r29
+#else
in r28,__SP_L__
in r29,__SP_H__
sub r28,r26
@@ -1238,6 +1254,8 @@ DEFUN __prologue_saves__
out __SP_H__,r29
out __SREG__,__tmp_reg__
out __SP_L__,r28
+#endif /* #SP = 8/16 */
+
#if defined (__AVR_HAVE_EIJMP_EICALL__)
eijmp
#else
@@ -1270,6 +1288,15 @@ DEFUN __epilogue_restores__
ldd r16,Y+4
ldd r17,Y+3
ldd r26,Y+2
+#if defined (__AVR_HAVE_8BIT_SP__)
+;; FIXME: __AVR_HAVE_8BIT_SP__ is set on device level, not on core level
+;; so this lines are dead code. To make it work, devices without
+;; SP_H must get their own multilib(s).
+ ldd r29,Y+1
+ add r28,r30
+ out __SP_L__,r28
+ mov r28, r26
+#else
ldd r27,Y+1
add r28,r30
adc r29,__zero_reg__
@@ -1280,6 +1307,7 @@ DEFUN __epilogue_restores__
out __SP_L__,r28
mov_l r28, r26
mov_h r29, r27
+#endif /* #SP = 8/16 */
ret
ENDF __epilogue_restores__
#endif /* defined (L_epilogue) */