This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch] Fix PR 60040
- From: Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>
- To: Bernd Schmidt <bschmidt at redhat dot com>
- Cc: Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, <uweigand at de dot ibm dot com>, <gnu at amylaar dot uk>
- Date: Fri, 15 Apr 2016 18:22:13 +0530
- Subject: Re: [Patch] Fix PR 60040
- Authentication-results: sourceware.org; auth=none
- References: <87h9fd1vqf dot fsf at atmel dot com> <5706B950 dot 3090003 at redhat dot com>
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);
+ }
+}