Bernd Schmidt writes: > On 04/07/2016 01:52 PM, Senthil Kumar Selvaraj wrote: >> The below patch fixes PR 60040 by not halting with a hard error on >> a spill failure, if reload knows that it has to run again anyway. > > Some additional information as to how this situation creates a spill > failure would be useful. It's hard to tell whether this patch just > papers over a problem that can still trigger in other circumstances.
For both testcases in the PR, reload fails to take into account that FP-SP elimination can no longer be performed, and tries to find reload regs for an rtx generated when FP-SP elimination was valid. 1. reload initializes elim table with FP->SP elimination enabled. 2. alter_reg for a pseudo allocates a stack slot for the pseudo, and sets reg_equiv_memory_loc to frame_pointer_rtx plus offset. It also sets something_was_spilled to true. 3. The main reload loop starts, and it resets something_was_spilled to false. 4. reload calls eliminate_regs for the pseudo and sets reg_equiv_address to (mem(SP + offset)). 5. calculate_needs_all_insns pushes a reload for SP (for the AVR target, SP cannot be a pointer reg - it needs to be reloaded into X Y or Z regs). 6. update_eliminables_and_spill calls targetm.frame_pointer_required, which returns true. That causes can_eliminate for FP->SP to be reset to zero, and FP to be added to bad_spill_regs_global. For the AVR, FP is Y, one of the 3 pointer regs. reload also notes that something has changed, and that the loop needs to run again. 7. reload still calls select_reload_regs, and find_regs fails to find a pointer reg to reload SP, which is unnecessary as FP->SP elimination had been disabled anyway in (6). IOW, reload fails to find pointer regs for an RTL expression that was created when FP->SP elimination was true, even after it turns out that the elimination can't be done after all. The patch tries to detect that - if it knows the loop is going to run again, it silences the failure. Also note that at a different point in the loop, the reload loop starts over if something_was_spilled (line 982-986). If set outside the reload loop by alter_reg, it gets reset at (3) - not sure why. I'd think a "continue" after update_eliminables_and_spill (line 1019-1022) would also work - haven't tested it though. What do you think? > >> - spill_failure (chain->insn, rld[r].rclass); >> - failure = 1; >> - return; >> + if (!tentative) >> + { >> + spill_failure (chain->insn, rld[r].rclass); >> + failure = 1; >> + return; >> + } >> } > > The indentation looks all wrong. > Fixed now - mixed up tabs and spaces. gcc/ChangeLog 2016-04-07 Joern Rennecke <g...@amylaar.uk> Senthil Kumar Selvaraj <senthil_kumar.selva...@atmel.com> PR target/60040 * reload1.c (find_reload_regs): Add tentative parameter. and don't report spill failure if param set. (reload): Propagate something_changed to select_reload_regs. (select_reload_regs): Add tentative parameter. gcc/testsuite/ChangeLog 2016-04-07 Sebastian Huber <sebastian.hu...@embedded-brains.de> Matthijs Kooijman <matth...@stdin.nl> Senthil Kumar Selvaraj <senthil_kumar.selva...@atmel.com> PR target/60040 * gcc.target/avr/pr60040-1.c: New. * gcc.target/avr/pr60040-2.c: Likewise. diff --git gcc/reload1.c gcc/reload1.c index c2800f8..58993a3 100644 --- gcc/reload1.c +++ gcc/reload1.c @@ -346,8 +346,8 @@ static void maybe_fix_stack_asms (void); static void copy_reloads (struct insn_chain *); static void calculate_needs_all_insns (int); static int find_reg (struct insn_chain *, int); -static void find_reload_regs (struct insn_chain *); -static void select_reload_regs (void); +static void find_reload_regs (struct insn_chain *, bool); +static void select_reload_regs (bool); static void delete_caller_save_insns (void); static void spill_failure (rtx_insn *, enum reg_class); @@ -1022,7 +1022,7 @@ reload (rtx_insn *first, int global) something_changed = 1; } - select_reload_regs (); + select_reload_regs (something_changed); if (failure) goto failed; @@ -1960,10 +1960,13 @@ find_reg (struct insn_chain *chain, int order) is given by CHAIN. Do it by ascending class number, since otherwise a reg might be spilled for a big class and might fail to count - for a smaller class even though it belongs to that class. */ + for a smaller class even though it belongs to that class. + TENTATIVE means that we had some changes that might have invalidated + the reloads and that we are going to loop again anyway, so don't give + a hard error on failure to find a reload reg. */ static void -find_reload_regs (struct insn_chain *chain) +find_reload_regs (struct insn_chain *chain, bool tentative) { int i; @@ -2012,9 +2015,12 @@ find_reload_regs (struct insn_chain *chain) { if (dump_file) fprintf (dump_file, "reload failure for reload %d\n", r); - spill_failure (chain->insn, rld[r].rclass); - failure = 1; - return; + if (!tentative) + { + spill_failure (chain->insn, rld[r].rclass); + failure = 1; + return; + } } } @@ -2025,14 +2031,14 @@ find_reload_regs (struct insn_chain *chain) } static void -select_reload_regs (void) +select_reload_regs (bool tentative) { struct insn_chain *chain; /* Try to satisfy the needs for each insn. */ for (chain = insns_need_reload; chain != 0; chain = chain->next_need_reload) - find_reload_regs (chain); + find_reload_regs (chain, tentative); } /* Delete all insns that were inserted by emit_caller_save_insns during diff --git gcc/testsuite/gcc.target/avr/pr60040-1.c gcc/testsuite/gcc.target/avr/pr60040-1.c new file mode 100644 index 0000000..4fe296b --- /dev/null +++ gcc/testsuite/gcc.target/avr/pr60040-1.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-Os" } */ + +float dhistory[10]; +float test; + +float getSlope(float history[]) { + float sumx = 0; + float sumy = 0; + float sumxy = 0; + float sumxsq = 0; + float rate = 0; + int n = 10; + + int i; + for (i=1; i< 11; i++) { + sumx = sumx + i; + sumy = sumy + history[i-1]; + sumy = sumy + history[i-1]; + sumxsq = sumxsq + (i*i); + } + + rate = sumy+sumx+sumxsq; + return rate; +} + +void loop() { + test = getSlope(dhistory); +} diff --git gcc/testsuite/gcc.target/avr/pr60040-2.c gcc/testsuite/gcc.target/avr/pr60040-2.c new file mode 100644 index 0000000..c40d49f --- /dev/null +++ gcc/testsuite/gcc.target/avr/pr60040-2.c @@ -0,0 +1,112 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +typedef unsigned char __uint8_t; +typedef short unsigned int __uint16_t; +typedef long unsigned int __uint32_t; +typedef __uint8_t uint8_t ; +typedef __uint16_t uint16_t ; +typedef __uint32_t uint32_t ; +typedef __builtin_va_list __gnuc_va_list; +typedef __gnuc_va_list va_list; +typedef enum rtems_blkdev_request_op { + RTEMS_BLKDEV_REQ_READ, +} rtems_fdisk_segment_desc; +typedef struct rtems_fdisk_driver_handlers +{ + int (*blank) (const rtems_fdisk_segment_desc* sd, + uint32_t device, + uint32_t segment, + uint32_t offset, + uint32_t size); +} rtems_fdisk_driver_handlers; +typedef struct rtems_fdisk_page_desc +{ + uint16_t flags; + uint32_t block; +} rtems_fdisk_page_desc; +typedef struct rtems_fdisk_segment_ctl +{ + rtems_fdisk_page_desc* page_descriptors; + uint32_t pages_active; +} rtems_fdisk_segment_ctl; +typedef struct rtems_fdisk_segment_ctl_queue +{ +} rtems_fdisk_segment_ctl_queue; +typedef struct rtems_fdisk_device_ctl +{ + uint32_t flags; + uint8_t* copy_buffer; +} rtems_flashdisk; + +extern void rtems_fdisk_error (const char *, ...); +extern int rtems_fdisk_seg_write(const rtems_flashdisk*, + rtems_fdisk_segment_ctl*, + uint32_t, + const rtems_fdisk_page_desc* page_desc, + uint32_t); + +void rtems_fdisk_printf (const rtems_flashdisk* fd, const char *format, ...) +{ + { + va_list args; + __builtin_va_start(args,format); + } +} +static int +rtems_fdisk_seg_blank_check (const rtems_flashdisk* fd, + rtems_fdisk_segment_ctl* sc, + uint32_t offset, + uint32_t size) +{ + uint32_t device; + uint32_t segment; + const rtems_fdisk_segment_desc* sd; + const rtems_fdisk_driver_handlers* ops; + return ops->blank (sd, device, segment, offset, size); +} +static int +rtems_fdisk_seg_write_page_desc (const rtems_flashdisk* fd, + rtems_fdisk_segment_ctl* sc, + uint32_t page, + const rtems_fdisk_page_desc* page_desc) +{ + uint32_t offset = page * sizeof (rtems_fdisk_page_desc); + if ((fd->flags & (1 << 3))) + { + int ret = rtems_fdisk_seg_blank_check (fd, sc, + offset, + sizeof (rtems_fdisk_page_desc)); + } + return rtems_fdisk_seg_write (fd, sc, offset, + page_desc, sizeof (rtems_fdisk_page_desc)); +} +void +rtems_fdisk_recycle_segment (rtems_flashdisk* fd, + rtems_fdisk_segment_ctl* ssc, + rtems_fdisk_segment_ctl* dsc, + uint32_t *pages) +{ + int ret; + uint32_t spage; + uint32_t used = 0; + uint32_t active = 0; + { + rtems_fdisk_page_desc* spd = &ssc->page_descriptors[spage]; + { + rtems_fdisk_page_desc* dpd; + uint32_t dpage; + dpd = &dsc->page_descriptors[dpage]; + *dpd = *spd; + ret = rtems_fdisk_seg_write_page_desc (fd, + dsc, + dpage, dpd); + } + } + rtems_fdisk_printf (fd, "ssc end: %d-%d: p=%ld, a=%ld, u=%ld", + pages, active, used); + { + rtems_fdisk_error ("compacting: ssc pages not 0: %d", + ssc->pages_active); + } +}