Fix PR 60040

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  <gnu@amylaar.uk>
            Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@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.huber@embedded-brains.de>
            Matthijs Kooijman  <matthijs@stdin.nl>
            Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

        PR target/60040
        * gcc.target/avr/pr60040-1.c: New.
        * gcc.target/avr/pr60040-2.c: Likewise.

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  <gnu@amylaar.uk>
>             Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@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.huber@embedded-brains.de>
>             Matthijs Kooijman  <matthijs@stdin.nl>
>             Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@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);
> +  }
> +}

On 04/15/2016 02:52 PM, Senthil Kumar Selvaraj wrote:
>
> 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.

That's what I was going to ask next. I think if that works for you, I 
think that's an approach that would make more sense.

However, it looks like after this call, and the other one in the same 
loop, should probably be calling finish_spills. It looks like an 
oversight in the existing code that this doesn't happen for the existing 
continue. Please try adding it in both places.


Bernd