| Message ID | 4ED4BEDE.4000600@gjlay.de |
|---|---|
| State | New |
| Headers | show |
2011/11/29 Georg-Johann Lay <avr@gjlay.de>: > 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. I'm agree. > > Ok to commit to 4.6? Approved. > > What about splitting multilibs? Seems that splitting multilibs is a right way. > Is this appropriate for 4.7? As I understand, any changes appropriate for our port in any stage. > 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. > Denis.
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. > Dropping this patch... There is ATtiny4313 (and maybe others) that have 8-bit SP and 0x100 RAM. As RAM starts at 0x60, I wonder what the meaning of SP is? Is it RAM-address % 0x100? Or offset to 0x60? Maybe Eric can clarify this? The sequence to read SP would then be something like IN %A0, __SP_L__ CPI %A0, 0x60 SBC %B0, %B0 NEG %B0 to reconstruct the hight part in the first case and IN %A0, __SP_L__ CPI %A0, 0xA0 SBC %B0, %B0 INC %B0 the the second. It might even need more instructions because CPI need D-reg but in movhi there is just general reg. Moreover, the data sheet just mentions SP_L but not SP_H. Yet the sample program from above disassembles to ... 0000002a <__ctors_end>: 2a: 11 24 eor r1, r1 2c: 1f be out 0x3f, r1 ; 63 2e: cf e5 ldi r28, 0x5F ; 95 30: d1 e0 ldi r29, 0x01 ; 1 32: de bf out 0x3e, r29 ; 62 34: cd bf out 0x3d, r28 ; 61 ... i.e. start-up code from avr-libc is initializing 0x3e (SP_H) with 1. Is this bug in avr-libc or typo in the manual? The "AVR538: Migrating from ATtiny2313 to ATtiny4313" application note does not address this either. Johann
As Georg-Johann Lay wrote: > There is ATtiny4313 (and maybe others) that have 8-bit SP and 0x100 RAM. > As RAM starts at 0x60, I wonder what the meaning of SP is? I think this is simply a bug in the datasheet. The device description XML file of the ATtiny4313 mentions an SPH register, and it would not make any much sense without it.
Joerg Wunsch wrote: > As Georg-Johann Lay wrote: > >> There is ATtiny4313 (and maybe others) that have 8-bit SP and 0x100 RAM. >> As RAM starts at 0x60, I wonder what the meaning of SP is? > > I think this is simply a bug in the datasheet. The device description > XML file of the ATtiny4313 mentions an SPH register, and it would not > make any much sense without it. Then avr-mcus.def adopted this bug from the manual for ATtiny4313 at least: AVR_MCU ("attiny4313", ARCH_AVR25, "__AVR_ATtiny4313__", 1 /* short_sp, should be 0 ? */, 0, 0x0060, "tn4313") Johann
As Georg-Johann Lay wrote: > Then avr-mcus.def adopted this bug from the manual for ATtiny4313 at least: > > AVR_MCU ("attiny4313", ARCH_AVR25, "__AVR_ATtiny4313__", 1 /* short_sp, should > be 0 ? */, 0, 0x0060, "tn4313") Not unlikely. I just ordered one. Hopefully, it will be here by tomorrow, so I can test it on a live device.
Joerg Wunsch wrote: > As Georg-Johann Lay wrote: > >> Then avr-mcus.def adopted this bug from the manual for ATtiny4313 at least: >> >> AVR_MCU ("attiny4313", ARCH_AVR25, "__AVR_ATtiny4313__", 1 /* short_sp, should >> be 0 ? */, 0, 0x0060, "tn4313") > > Not unlikely. > > I just ordered one. Hopefully, it will be here by tomorrow, so I can > test it on a live device. Jörg, do you have an easy way to review avr-mcus.def? http://gcc.gnu.org/viewcvs/trunk/gcc/config/avr/avr-mcus.def?content-type=text%2Fplain&view=co If you have XML hardware descriptions that are more accurate and much more easy to use than 1E2...1E3 PDFs that might be not too painful. The column in question is the column after the built-in define definition like "__AVR_AT90S2313__" i.e. the 4th column. Johann
Index: config/avr/libgcc.S =================================================================== --- config/avr/libgcc.S (revision 181783) +++ config/avr/libgcc.S (working copy) @@ -582,6 +582,15 @@ __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 + clr r29 + out __SP_L__,r28 +#else in r28,__SP_L__ in r29,__SP_H__ sub r28,r26 @@ -591,6 +600,7 @@ __prologue_saves__: out __SP_H__,r29 out __SREG__,__tmp_reg__ out __SP_L__,r28 +#endif #if defined (__AVR_HAVE_EIJMP_EICALL__) eijmp #else @@ -625,6 +635,15 @@ __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__ @@ -635,6 +654,7 @@ __epilogue_restores__: out __SP_L__,r28 mov_l r28, r26 mov_h r29, r27 +#endif ret .endfunc #endif /* defined (L_epilogue) */ Index: config/avr/avr.md =================================================================== --- config/avr/avr.md (revision 181783) +++ config/avr/avr.md (working copy) @@ -299,7 +299,7 @@ (define_insn "movhi_sp_r_irq_off" [(set (match_operand:HI 0 "stack_register_operand" "=q") (unspec_volatile:HI [(match_operand:HI 1 "register_operand" "r")] UNSPECV_WRITE_SP_IRQ_OFF))] - "" + "!AVR_HAVE_8BIT_SP" "out __SP_H__, %B1 out __SP_L__, %A1" [(set_attr "length" "2") @@ -309,7 +309,7 @@ (define_insn "movhi_sp_r_irq_on" [(set (match_operand:HI 0 "stack_register_operand" "=q") (unspec_volatile:HI [(match_operand:HI 1 "register_operand" "r")] UNSPECV_WRITE_SP_IRQ_ON))] - "" + "!AVR_HAVE_8BIT_SP" "cli out __SP_H__, %B1 sei Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 181783) +++ config/avr/avr.c (working copy) @@ -1879,9 +1879,12 @@ output_movhi (rtx insn, rtx operands[], } else if (test_hard_reg_class (STACK_REG, src)) { - *l = 2; - return (AS2 (in,%A0,__SP_L__) CR_TAB - AS2 (in,%B0,__SP_H__)); + *l = 2; + return AVR_HAVE_8BIT_SP + ? (AS2 (in,%A0,__SP_L__) CR_TAB + AS1 (clr,%B0)) + : (AS2 (in,%A0,__SP_L__) CR_TAB + AS2 (in,%B0,__SP_H__)); } if (AVR_HAVE_MOVW) @@ -5173,10 +5176,10 @@ avr_file_start (void) default_file_start (); -/* fprintf (asm_out_file, "\t.arch %s\n", avr_mcu_name);*/ - fputs ("__SREG__ = 0x3f\n" - "__SP_H__ = 0x3e\n" - "__SP_L__ = 0x3d\n", asm_out_file); + fputs ("__SREG__ = 0x3f\n", asm_out_file); + if (!AVR_HAVE_8BIT_SP) + fputs ("__SP_H__ = 0x3e\n", asm_out_file); + fputs ("__SP_L__ = 0x3d\n", asm_out_file); fputs ("__tmp_reg__ = 0\n" "__zero_reg__ = 1\n", asm_out_file);
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.