From patchwork Fri Apr 15 12:52:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Senthil Kumar Selvaraj X-Patchwork-Id: 610961 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3qmcrk3cTHz9t6T for ; Fri, 15 Apr 2016 22:52:38 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=iiUllE5O; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :references:from:to:cc:subject:in-reply-to:date:message-id :mime-version:content-type; q=dns; s=default; b=Q04vRuJvkXtJHdgI 1t4ns8uFxM5d0f2RXTHX4qC7R3JYJU4mkrenY32rv9mcWWR1X0AWCBsFAYDE97bx cpKSBiRq/jNIuZAm0U4FIVTGV9RfN+xyYBrpQglbHXwF70DGXi676r5mJ944/AVH JkYzs9BBSIol7ACQfH+lkw1ok6c= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :references:from:to:cc:subject:in-reply-to:date:message-id :mime-version:content-type; s=default; bh=RAsl1hTV7L5D6VDCabSumq Yan40=; b=iiUllE5Ot46XxaUpPm30c2mSLYTr5Ty1XuJihTELdEJp3Z2SlrFMZY fgrawCwXi6QovrV3OqdjStt4Ui+BnmZKzNZ7jiIxFZxsaquPYuE1cTUtLCAldCc4 mMNJ34J7cSrHV2oFzmz1UDPCJHvbUlfr7naiiRiQmr66r1rYlzMR4= Received: (qmail 83013 invoked by alias); 15 Apr 2016 12:52:21 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 82766 invoked by uid 89); 15 Apr 2016 12:52:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=BAYES_20, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=tabs, rclass, rtx_insn, reg_class X-HELO: eusmtp01.atmel.com Received: from eusmtp01.atmel.com (HELO eusmtp01.atmel.com) (212.144.249.242) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 15 Apr 2016 12:52:00 +0000 Received: from HNOCHT02.corp.atmel.com (10.161.30.162) by eusmtp01.atmel.com (10.161.101.30) with Microsoft SMTP Server (TLS) id 14.3.235.1; Fri, 15 Apr 2016 14:51:50 +0200 Received: from jaguar.atmel.com (10.161.30.18) by HNOCHT02.corp.atmel.com (10.161.30.162) with Microsoft SMTP Server (TLS) id 14.3.235.1; Fri, 15 Apr 2016 14:51:54 +0200 References: <87h9fd1vqf.fsf@atmel.com> <5706B950.3090003@redhat.com> User-agent: mu4e 0.9.17; emacs 24.5.1 From: Senthil Kumar Selvaraj To: Bernd Schmidt CC: Senthil Kumar Selvaraj , "gcc-patches@gcc.gnu.org" , , Subject: Re: [Patch] Fix PR 60040 In-Reply-To: <5706B950.3090003@redhat.com> Date: Fri, 15 Apr 2016 18:22:13 +0530 Message-ID: <8737qnrq4i.fsf@atmel.com> MIME-Version: 1.0 X-IsSubscribed: yes 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 Senthil Kumar Selvaraj 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 Matthijs Kooijman Senthil Kumar Selvaraj 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); + } +}