From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15583 invoked by alias); 8 Jul 2011 10:04:25 -0000 Received: (qmail 15573 invoked by uid 22791); 8 Jul 2011 10:04:24 -0000 X-SWARE-Spam-Status: No, hits=0.0 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE X-Spam-Check-By: sourceware.org Received: from mo-p00-ob.rzone.de (HELO mo-p00-ob.rzone.de) (81.169.146.160) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 08 Jul 2011 10:04:05 +0000 X-RZG-AUTH: :LXoWVUeid/7A29J/hMvvT2k715jHQaJercGObUOFkj18odoYNahU4Q== X-RZG-CLASS-ID: mo00 Received: from [192.168.0.22] (business-188-111-022-002.static.arcor-ip.net [188.111.22.2]) by smtp.strato.de (klopstock mo57) (RZmta 26.0) with ESMTPA id w0720cn689hfiI ; Fri, 8 Jul 2011 11:59:26 +0200 (MEST) Message-ID: <4E16D4FD.8020806@gjlay.de> Date: Fri, 08 Jul 2011 10:12:00 -0000 From: Georg-Johann Lay User-Agent: Thunderbird 2.0.0.24 (X11/20100302) MIME-Version: 1.0 To: Denis Chertykov CC: Richard Henderson , gcc-patches@gcc.gnu.org, Anatoly Sokolov , "Eric B. Weddington" , Eric Botcazou , Bernd Schmidt Subject: Re: [Patch, AVR]: Fix PR46779 References: <4DF0FAB5.6070704@gjlay.de> <4DF11D20.4030907@gjlay.de> <4DF1ED76.4030507@gjlay.de> <4DF650B7.3030705@gjlay.de> <4DF73490.2080709@gjlay.de> <4DF7D2B5.1090708@gjlay.de> <4DF8ED42.1030706@redhat.com> <4DF918A9.4070003@gjlay.de> <4DF92AEA.4000906@redhat.com> <4DF93B17.8020008@redhat.com> <4E03B658.8020509@redhat.com> <4E078F93.7060901@gjlay.de> <4E084291.4030300@gjlay.de> <4E157B72.1000304@gjlay.de> In-Reply-To: Content-Type: multipart/mixed; boundary="------------010709020201050107070607" X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-07/txt/msg00607.txt.bz2 This is a multi-part message in MIME format. --------------010709020201050107070607 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-length: 3205 CCed Eric and Bernd. Denis Chertykov wrote: >> Did you decide about the fix for PR46779? >> >> http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00810.html >> >> Is it ok to commit? > > I forgot about testsuite regressions for this patch. > > Denis. There were no new regressions: http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00747.html However, with the actual trunk (SVN 175991), I get two more spill fails for following sources: ./gcc.c-torture/compile/pr32349.c -O1 -mmcu=atmega128 pr30338.c: In function 'testload_func': pr30338.c:13:1: error: unable to find a register to spill in class 'POINTER_REGS' pr30338.c:13:1: error: this is the insn: (insn 14 13 15 2 (set (reg:QI 24 r24 [orig:73 *D.1963_37 ] [73]) (mem:QI (subreg:HI (reg:SI 71) 0) [0 *D.1963_37+0 S1 A8])) pr30338.c:9 4 {*movqi} (expr_list:REG_DEAD (reg:SI 71) (nil))) pr30338.c:13:1: internal compiler error: in spill_failure, at reload1.c:2120 ./gcc.c-torture/compile/pr32349.c -S -O3 -funroll-loops pr32349.c: In function 'foo': pr32349.c:26:1: error: unable to find a register to spill in class 'POINTER_REGS' pr32349.c:26:1: error: this is the insn: (insn 175 197 177 10 (set (reg/v:SI 234 [ m ]) (mem:SI (post_inc:HI (reg:HI 16 r16 [orig:192 ivtmp.18 ] [192])) [3 MEM[base: D.1996_74, offset: 0B]+0 S4 A8])) pr32349.c:18 12 {*movsi} (expr_list:REG_INC (reg:HI 16 r16 [orig:192 ivtmp.18 ] [192]) (nil))) pr32349.c:26:1: internal compiler error: in spill_failure, at reload1.c:2120 (1) I can fix *both* fails with additional test in avr_hard_regno_mode_ok: + if (GET_MODE_SIZE (mode) >= 4 + && regno >= REG_X) + return 0; (2) I can fix the first fail but *not* the second by not allow SUBREGs in avr_legitimate_address_p: - if (!strict && GET_CODE (x) == SUBREG) */ - x = SUBREG_REG (x); */ (2) Looks very reasonble, Eric Botcazou proposed it because he ran into problems: http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01367.html (1) Appears to be hackish, but it should be ok. If code breaks because of that is's *definitely* a reload bug (e.g. SI-subreg of DI). Even the original avr_hard_regno_mode_ok is ok IMO because if a machine says "I can hold HI in 28 but not QI in 29" reload has to handle it (except a machine must allow word_mode in *all* it's GENERAL_REGS, don't know if that's a must). I made a patch for reload, too: http://gcc.gnu.org/ml/gcc/2011-06/msg00005.html Because IRA generates SUBREG of hardreg (which old lreg/greg handled ok) and reload does not handle it correctly. It generates a spill but without the needed input reload so that one part of the register is missing. reload blames IRA or BE, IRA blames reload, BE blames IRA, etc... I didn't rerun the testsuite with (1) or/and (2), I'd like both (1) and (2) in the compiler. What do you think? For reference, I attached the patch again. It's like the original patch, just with some comment change. Johann PR target/46779 * config/avr/avr.c (avr_hard_regno_mode_ok): Rewrite. In particular, allow 8-bit values in r28 and r29. (avr_hard_regno_scratch_ok): Disallow any register that might be part of the frame pointer. (avr_hard_regno_rename_ok): Same. --------------010709020201050107070607 Content-Type: text/x-patch; name="pr46779.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="pr46779.diff" Content-length: 3016 Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 175991) +++ config/avr/avr.c (working copy) @@ -6118,26 +6118,21 @@ jump_over_one_insn_p (rtx insn, rtx dest int avr_hard_regno_mode_ok (int regno, enum machine_mode mode) { - /* Disallow QImode in stack pointer regs. */ - if ((regno == REG_SP || regno == (REG_SP + 1)) && mode == QImode) - return 0; - - /* The only thing that can go into registers r28:r29 is a Pmode. */ - if (regno == REG_Y && mode == Pmode) - return 1; - - /* Otherwise disallow all regno/mode combinations that span r28:r29. */ - if (regno <= (REG_Y + 1) && (regno + GET_MODE_SIZE (mode)) >= (REG_Y + 1)) - return 0; - - if (mode == QImode) + /* NOTE: 8-bit values must not be disallowed for R28 or R29. + Disallowing QI et al. in these regs might lead to code like + (set (subreg:QI (reg:HI 28) n) ...) + which will result in wrong code because reload does not + handle SUBREGs of hard regsisters like this. + This could be fixed in reload. However, it appears + that fixing reload is not wanted by reload people. */ + + /* Any GENERAL_REGS register can hold 8-bit values. */ + + if (GET_MODE_SIZE (mode) == 1) return 1; - - /* Modes larger than QImode occupy consecutive registers. */ - if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER) - return 0; - - /* All modes larger than QImode should start in an even register. */ + + /* All modes larger than 8 bits should start in an even register. */ + return !(regno & 1); } @@ -6410,13 +6405,23 @@ avr_hard_regno_scratch_ok (unsigned int && !df_regs_ever_live_p (regno)) return false; + /* Don't allow hard registers that might be part of the frame pointer. + Some places in the compiler just test for [HARD_]FRAME_POINTER_REGNUM + and don't care for a frame pointer that spans more than one register. */ + + if ((!reload_completed || frame_pointer_needed) + && (regno == REG_Y || regno == REG_Y + 1)) + { + return false; + } + return true; } /* Return nonzero if register OLD_REG can be renamed to register NEW_REG. */ int -avr_hard_regno_rename_ok (unsigned int old_reg ATTRIBUTE_UNUSED, +avr_hard_regno_rename_ok (unsigned int old_reg, unsigned int new_reg) { /* Interrupt functions can only use registers that have already been @@ -6427,6 +6432,17 @@ avr_hard_regno_rename_ok (unsigned int o && !df_regs_ever_live_p (new_reg)) return 0; + /* Don't allow hard registers that might be part of the frame pointer. + Some places in the compiler just test for [HARD_]FRAME_POINTER_REGNUM + and don't care for a frame pointer that spans more than one register. */ + + if ((!reload_completed || frame_pointer_needed) + && (old_reg == REG_Y || old_reg == REG_Y + 1 + || new_reg == REG_Y || new_reg == REG_Y + 1)) + { + return 0; + } + return 1; } --------------010709020201050107070607--