From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5929 invoked by alias); 8 Jul 2011 12:03:46 -0000 Received: (qmail 5918 invoked by uid 22791); 8 Jul 2011 12:03:43 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,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.161) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 08 Jul 2011 12:03:27 +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 post.strato.de (mrclete mo31) (RZmta 26.0) with ESMTPA id e0143fn68B7NWE ; Fri, 8 Jul 2011 13:59:20 +0200 (MEST) Message-ID: <4E16F116.7060809@gjlay.de> Date: Fri, 08 Jul 2011 12:16: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> <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> <4E16D4FD.8020806@gjlay.de> In-Reply-To: Content-Type: multipart/mixed; boundary="------------030903020306060900020405" 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/msg00625.txt.bz2 This is a multi-part message in MIME format. --------------030903020306060900020405 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-length: 5959 Denis Chertykov wrote: > 2011/7/8 Georg-Johann Lay : >> 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? > > I think that AVR is a stress test for GCC core. We are on the edge. > IMHO your patch is a change one tweaks to another. > It's not needed if it adds regressions. > > Denis. Reran testsuite against newer version (175991). First the good news. Following tests pass, that's the reason for the patch: * gcc.target/avr/pr46779-1.c * gcc.target/avr/pr46779-2.c These tests now pass, too. They all came up with spill fail both with and without original patch, but pass with (1) and (2) added: * gcc.c-torture/execute/pr38051.c (-Os) * gcc.dg/20030324-1.c (-O -fstrict-aliasing -fgcse) * gcc.dg/pr43670.c (-O -ftree-vrp -fcompare-debug) And here the not-so-good news. There's additional ICE in reload: * gcc.dg/pr32912-2.c (-Os) pr32912-2.c:23:1: internal compiler error: in find_valid_class, at reload.c:708 Please submit a full bug report, with preprocessed source if appropriate. See for instructions. compiler exited with status 1 output is: pr32912-2.c: In function 'bar': pr32912-2.c:23:1: internal compiler error: in find_valid_class, at reload.c:708 But look at the source! #if(__SIZEOF_INT__ >= 4) typedef int __m128i __attribute__ ((__vector_size__ (16))); #else typedef long __m128i __attribute__ ((__vector_size__ (16))); #endif That's no sensible on AVR at all! The stack trace: Breakpoint 1, fancy_abort (file=0x8896f38 "../../../gcc.gnu.org/trunk/gcc/reload.c", line=708, function=0x8896fa8 "find_valid_class") at ../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893 (gdb) bt #0 fancy_abort (file=0x8896f38 "../../../gcc.gnu.org/trunk/gcc/reload.c", line=708, function=0x8896fa8 "find_valid_class") at ../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893 #1 0x0845a9d4 in find_valid_class (outer=SImode, inner=TImode, n=12, dest_regno=16) at ../../../gcc.gnu.org/trunk/gcc/reload.c:708 #2 0x0845bfd5 in push_reload (in=0x0, out=0xb7dfe2c4, inloc=0x0, outloc=0xb7dfe2d4, rclass=GENERAL_REGS, inmode=VOIDmode, outmode=SImode, strict_low=0, optional=0, opnum=0, type=RELOAD_FOR_OUTPUT) at ../../../gcc.gnu.org/trunk/gcc/reload.c:1196 #3 0x08462d52 in find_reloads (insn=0xb7dff144, replace=0, ind_levels=0, live_known=1, reload_reg_p=0x89607c0) at ../../../gcc.gnu.org/trunk/gcc/reload.c:4015 #4 0x08470af9 in calculate_needs_all_insns (global=1) at ../../../gcc.gnu.org/trunk/gcc/reload1.c:1525 Note the TImode, this ICE is no harm for AVR! As gcc.dg/pr32912-2.c is completely irrelevant for AVR, here is the patch for review that integrates (1) and (2). Ok? 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. (avr_legitimate_address_p): Don't allow SUBREGs. --------------030903020306060900020405 Content-Type: text/x-patch; name="pr46779.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="pr46779.diff" Content-length: 3648 Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 175991) +++ config/avr/avr.c (working copy) @@ -1109,8 +1109,7 @@ avr_legitimate_address_p (enum machine_m true_regnum (XEXP (x, 0))); debug_rtx (x); } - if (!strict && GET_CODE (x) == SUBREG) - x = SUBREG_REG (x); + if (REG_P (x) && (strict ? REG_OK_FOR_BASE_STRICT_P (x) : REG_OK_FOR_BASE_NOSTRICT_P (x))) r = POINTER_REGS; @@ -6118,26 +6117,30 @@ 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) + /* FIXME: Ideally, the following test is not needed. + However, it turned out that it can reduce the number + of spill fails. AVR and it's poor endowment with + address registers is extreme stress test for reload. */ + + if (GET_MODE_SIZE (mode) >= 4 + && regno >= REG_X) 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 +6413,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 +6440,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; } --------------030903020306060900020405--