Ping! Regards Senthil
Senthil Kumar Selvaraj writes: > 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); > + } > +}