Bug 51345

Summary: [avr] Devices with 8-bit SP need their own multilib(s)
Product: gcc Reporter: Georg-Johann Lay <gjl>
Component: targetAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: eric.weddington
Priority: P3 Keywords: FIXME, wrong-code
Version: 4.6.2   
Target Milestone: 4.7.0   
Host: Target: avr
Build: Known to work:
Known to fail: Last reconfirmed: 2011-11-29 00:00:00
Bug Depends on: 52737    
Bug Blocks: 51002    
Attachments: tentative patch

Description Georg-Johann Lay 2011-11-29 14:28:01 UTC
Code that reads SP register must not read from the high byte of SP if the SP is just 8 bits wide.

The C code will be correct once PR51002 will have been fixed, however, there is code in libgcc that also reads from __SP_H__ like __prologue_saves__ and __epilogue_restores__.

The issue is that the 8-bit property is set on the device level in avr_current_device->short_sp and devices with different settings of short_sp are mixed up the the same multilib.
Comment 1 Georg-Johann Lay 2011-12-02 19:14:20 UTC
Author: gjl
Date: Fri Dec  2 19:14:15 2011
New Revision: 181936

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181936
Log:
	PR target/51002
	PR target/51345
	* 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.
	* config/avr/avr-devices.c (avr_mcu_types): ATtiny4313 and
	AT86RF401 have a 16-bit SP (their manual is bogus).


Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/avr/avr-devices.c
    branches/gcc-4_6-branch/gcc/config/avr/avr.c
    branches/gcc-4_6-branch/gcc/config/avr/avr.md
    branches/gcc-4_6-branch/gcc/config/avr/libgcc.S
Comment 2 Georg-Johann Lay 2011-12-06 15:04:17 UTC
Author: gjl
Date: Tue Dec  6 15:04:09 2011
New Revision: 182052

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182052
Log:
libgcc/
	Forward-port from gcc-4_6-branch r181936 2011-12-02.

	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/
	Forward-port from gcc-4_6-branch r181936 2011-12-02.

	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.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.c
    trunk/gcc/config/avr/avr.md
    trunk/libgcc/ChangeLog
    trunk/libgcc/config/avr/lib1funcs.S
Comment 3 Georg-Johann Lay 2011-12-28 12:21:40 UTC
Created attachment 26194 [details]
tentative patch
Comment 4 Georg-Johann Lay 2012-01-02 12:52:00 UTC
Author: gjl
Date: Mon Jan  2 12:51:57 2012
New Revision: 182797

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182797
Log:
contrib/
	PR target/51345
	* gcc_update (files_and_dependencies): Add
	gcc/config/avr/t-multilib, gcc/config/avr/multilib.h.
	
libgcc/
	PR target/51345
	* config/avr/lib1funcs.S: Remove FIXME comments.
	(SPEED_DIV): Depend on __AVR_HAVE_8BIT_SP__.
gcc/	
	PR target/51345
	* config.gcc (tm_file target=avr]): Add avr/avr-multilib.h
	(tmake_file target=avr): Add avr/t-multilib.

	* config/avr/avr-c.c (avr_cpu_cpp_builtins): Use AVR_HAVE_8BIT_SP
	to built-in define __AVR_HAVE_8BIT_SP__, __AVR_HAVE_16BIT_SP__.
	* config/avr/genmultilib.awk: New file.
	* config/avr/t-multilib: New auto-generated file.
	* config/avr/multilib.h: New auto-generated file.
	* config/avr/t-avr (AVR_MCUS): New variable.
	(genopt.sh): Use it.
	(s-mlib): Depend on t-multilib.
	(t-multilib, multilib.h): New dependencies.
	(s-avr-mlib): New rule to build t-multilib, multilib.h from AVR_MCUS.
	(MULTILIB_OPTIONS): Remove.
	(MULTILIB_MATCHES): Remove.
	(MULTILIB_DIRNAMES): Remove.
	(MULTILIB_EXCEPTIONS): Remove:
	* config/avr/genopt.sh: Don't use hard coded file name;
	pass AVR_MCUS from t-avr instead.


Added:
    trunk/gcc/config/avr/genmultilib.awk
    trunk/gcc/config/avr/multilib.h
    trunk/gcc/config/avr/t-multilib
Modified:
    trunk/contrib/ChangeLog
    trunk/contrib/gcc_update
    trunk/gcc/ChangeLog
    trunk/gcc/config.gcc
    trunk/gcc/config/avr/avr-c.c
    trunk/gcc/config/avr/avr-mcus.def
    trunk/gcc/config/avr/genopt.sh
    trunk/gcc/config/avr/t-avr
    trunk/libgcc/ChangeLog
    trunk/libgcc/config/avr/lib1funcs.S
Comment 5 Georg-Johann Lay 2012-01-04 15:29:29 UTC
Fixed in 4.7.0
Comment 6 Georg-Johann Lay 2012-03-24 21:06:42 UTC
Reopened:

The current (4.7.0) behaviour is:

-mtiny-stack acts as multilib-option for 16-bit SP targets in avr2 and avr25, i.e. with that switch will trigger multi-lib from ./tiny-stack resp. ./avr25/tiny-stack

The problem is that the tiny-stack multilibs mix 16-bit SP targets that are treated as 8-bit SP, and targets with 8-bit SP.

The currect implementation assumes SP.H = 0 in tiny-stack mlibs in order to properly initialize the frame pointer, e.g. in __prologue_saves.

This is no good for 16-bit SP targets.

Solution:

Restore the behaviour for the 16-bit SP targets as was, i.e. -mtiny-stack does not affect multilib selection for these targets, but if specified on the command line, the resulting code will only change SP.L and assume SP.H never changes.

For the 8-bit SP targets, -mtiny-stack should remain as in 4.7.0, i.e. used internally to trigger tiny-stack multilibs. Specifying -mtiny-stack on the command line will be redundant for these targets and have no effect.
Comment 7 Georg-Johann Lay 2012-03-28 10:29:40 UTC
Rescheduled for 4.7.1
Comment 8 Georg-Johann Lay 2012-05-31 17:32:48 UTC
Author: gjl
Date: Thu May 31 17:32:42 2012
New Revision: 188070

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188070
Log:
	PR target/51345
	* config/avr/avr.opt (-msp8): Document it.
	* doc/invoke.texi (AVR Options): Ditto.  And document related
	built-in macros.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.opt
    trunk/gcc/doc/invoke.texi