Bug 52484 - [avr]: Missing __memx insn because of wrong register footprint
Summary: [avr]: Missing __memx insn because of wrong register footprint
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.7.0
: P4 normal
Target Milestone: 4.7.1
Assignee: Georg-Johann Lay
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2012-03-04 19:50 UTC by Georg-Johann Lay
Modified: 2012-03-22 15:33 UTC (History)
0 users

See Also:
Host:
Target: avr
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-03-07 00:00:00


Attachments
flash.c.208r.peephole2 (1.92 KB, text/plain)
2012-03-04 19:54 UTC, Georg-Johann Lay
Details
flash.c.209r.ce3 (2.34 KB, text/plain)
2012-03-04 19:55 UTC, Georg-Johann Lay
Details
flash.c (56 bytes, text/plain)
2012-03-04 19:57 UTC, Georg-Johann Lay
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Georg-Johann Lay 2012-03-04 19:50:19 UTC
The following code for AVR target that reads from address space __memx result in wrong code:

long readx (const __memx long *p)
{
    return *p;
}

== compile ==

$ avr-gcc flash.c -S -dp -Os -mmcu=avr51 -da

== configure ==

../../gcc.gnu.org/trunk/configure --target=avr --prefix=/gnu/install/gcc-4.7 --disable-nls --with-dwarf2 --enable-checking=yes,rtl --enable-languages=c,c++

GNU C (GCC) version 4.8.0 20120304 (experimental) (avr)

SVN 184887 from 2012-03-04

To see the bug RTL dumps will follow.

There is really bloaty code from lower-subreg that splits the 4-byte move into 4 individual byte moves. 

After pass .peephole2 there are 4 xload_qi_libgcc insns that perform these moves.

After pass .ce3 one of these moves is gone, this is wrong because no byte must be thrown away.
Comment 1 Georg-Johann Lay 2012-03-04 19:54:20 UTC
Created attachment 26824 [details]
flash.c.208r.peephole2

This dump looks ok: there are 4 xload_qi_libgcc insns.

peep2 did no optimizations except transform one 

(set (reg/f:PSI 12 r12 [58])
     (const_int 2 [0x2]))

to 

(parallel [(set (reg/f:PSI 12 r12 [58])
                (const_int 2 [0x2]))
           (clobber (reg:QI 18 r18))])

which is ok (r18 is marked as dead above)
Comment 2 Georg-Johann Lay 2012-03-04 19:55:31 UTC
Created attachment 26825 [details]
flash.c.209r.ce3

One xload_qi_libgcc has been killed.
Comment 3 Georg-Johann Lay 2012-03-04 19:57:01 UTC
Created attachment 26826 [details]
flash.c
Comment 4 Georg-Johann Lay 2012-03-07 13:46:47 UTC
ce3 is correct in deleting the insn.

The bug is that "xload<mode>_A" insn needs a clobber of r22 to represent the register footprint of the libcall generated in split1.
Comment 5 Georg-Johann Lay 2012-03-07 13:52:37 UTC
Author: gjl
Date: Wed Mar  7 13:52:30 2012
New Revision: 185043

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185043
Log:
	PR target/52484
	* config/avr/avr.md (xload<mode>_A): Add R22... to register footprint.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.md
Comment 6 Georg-Johann Lay 2012-03-22 15:07:38 UTC
Author: gjl
Date: Thu Mar 22 15:06:57 2012
New Revision: 185697

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185697
Log:
libgcc/
	Backport from 2012-03-07 mainline r185033.

	PR target/52507
	* config/avr/lib1funcs.S (__movmemx_hi): Fix loop label in RAM-part.

	Backport from 2012-03-07 mainline r185031.

	PR target/52505
	* config/avr/lib1funcs.S (__xload_1): Don't read unintentionally
	from RAM.

	Backport from 2012-03-07 mainline r185030.

	PR target/52461
	PR target/52508
	* 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.
	(__movmemx_hi): Ditto.

gcc/
	Backport from 2012-03-22 mainline r185692.

	PR target/52496
	* config/avr/avr.md (unspec): Remove UNSPEC_MEMORY_BARRIER.
	(unspecv): Add UNSPECV_MEMORY_BARRIER.
	(cli_sei): Use unspec_volatile instead of unspec for memory barrier.
	(delay_cycles_1, delay_cycles_2): Ditto.
	(delay_cycles_3, delay_cycles_4): Ditto.
	(nopv, *nopv): Ditto.
	(sleep, *sleep): Ditto.
	(wdr, *wdr): Ditto.

	Backport from 2012-03-21 mainline r185605.

	PR rtl-optimization/52543
	PR target/52461
	* config/avr/avr-protos.h (avr_load_lpm): New prototype.
	* config/avr/avr.c (avr_mode_dependent_address_p): New function.
	(TARGET_MODE_DEPENDENT_ADDRESS_P): New define.
	(avr_load_libgcc_p): Restrict to __flash loads.
	(avr_out_lpm): Only handle 1-byte loads from __flash.
	(avr_load_lpm): New function.
	(avr_find_unused_d_reg): Remove.
	(avr_out_lpm_no_lpmx): Remove.
	(adjust_insn_length): Handle ADJUST_LEN_LOAD_LPM.
	* config/avr/avr.md (unspec): Add UNSPEC_LPM.
	(load_<mode>_libgcc): Use UNSPEC_LPM instead of MEM.
	(load_<mode>, load_<mode>_clobber): New insns.
	(mov<mode>): For multi-byte move from non-generic
	16-bit address spaces: Expand to load_<mode> resp.
	load_<mode>_clobber.
	(load<mode>_libgcc): Remove expander.
	(split-lpmx): Remove split.

	Backport from 2012-03-13 mainline r185329.

	PR target/52488
	* config/avr/avr.c (avr_prologue_setup_frame): Cut down stack
	offset (size) to a value the insns can deal with.
	(expand_epilogue): Ditto.

	Backport from 2012-03-12 mainline r185256.

	PR target/52499
	* config/avr/avr.c (avr_mode_code_base_reg_class): Change return
	type from reg_class_t to enum reg_class.
	* config/avr/avr-protos.h (avr_mode_code_base_reg_class): Ditto.

	Backport from 2012-03-12 mainline r185253.

	PR target/52148
	* config/avr/avr.c (avr_out_movmem): Fix typo in output template
	for the case ADDR_SPACE_FLASH and AVR_HAVE_LPMX introduced in
	r184615 from 2012-02-28.

	Backport from 2012-03-08 mainline r185105.

	* config/avr/avr.md (*addhi3, addhi3_clobber): Add "w" alternative
	for constants in [-63,63].

	Backport from 2012-03-08 mainline r185100.

	PR target/52496
	* config/avr/avr.c (avr_mem_clobber): New static function.
	(avr_expand_delay_cycles): Add memory clobber operand to
	delay_cycles_1, delay_cycles_2, delay_cycles_3, delay_cycles_4.
	* config/avr/avr.md (unspec): Add UNSPEC_MEMORY_BARRIER.
	(enable_interrupt, disable_interrupt): New expander.
	(nopv, sleep, wdr): New expanders.
	(delay_cycles_1): Add memory clobber.
	(delay_cycles_2): Add memory clobber.
	(delay_cycles_3): Add memory clobber.
	(delay_cycles_4): Add memory clobber.
	(cli_sei): New insn from former "enable_interrupt",
	"disable_interrupt" with memory clobber.
	(*wdt): New insn from former "wdt" with memory clobber.
	(*nopv): Similar, but for "nopv".
	(*sleep): Similar, but for "sleep".

	Backport from 2012-03-07 mainline r185043.

	PR target/52484
	* config/avr/avr.md (xload<mode>_A): Add R22... to register footprint.

	Backport from 2012-03-07 mainline r185032.

	PR target/52506
	* gcc/config/avr/avr.c (expand_epilogue): Fix order of restoration
	to: RAMPZ, RAMPY, RAMPX, RAMPD.
	(expand_prologue): Only clear RAMPZ if it has effect on RAM-read.

	Backport from 2012-03-07 mainline r185031.

	PR target/52505
	* config/avr/avr.c (avr_out_xload): Don't read unintentionally
	from RAM.
	* config/avr/avr.md (xload_8): Adjust insn length.

	Backport from 2012-03-07 mainline r185030.

	PR target/52461
	* gcc/config/avr/avr.c (avr_out_lpm): Clear RAMPZ after usage
	if RAMPZ affects reading from RAM.

	Backport from 2012-03-05 mainline r184919.

	* config/avr/avr.md (*umaddqihi4.2): New insn-and-split.


Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/avr/avr-protos.h
    branches/gcc-4_7-branch/gcc/config/avr/avr.c
    branches/gcc-4_7-branch/gcc/config/avr/avr.md
    branches/gcc-4_7-branch/libgcc/ChangeLog
    branches/gcc-4_7-branch/libgcc/config/avr/lib1funcs.S
Comment 7 Georg-Johann Lay 2012-03-22 15:33:19 UTC
Fixed in 4.7.1