From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17582 invoked by alias); 19 Apr 2011 10:55:45 -0000 Received: (qmail 17573 invoked by uid 22791); 19 Apr 2011 10:55:44 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST X-Spam-Check-By: sourceware.org Received: from mail-wy0-f175.google.com (HELO mail-wy0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 19 Apr 2011 10:55:30 +0000 Received: by wye20 with SMTP id 20so5661846wye.20 for ; Tue, 19 Apr 2011 03:55:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.227.169.140 with SMTP id z12mr6265852wby.89.1303210529157; Tue, 19 Apr 2011 03:55:29 -0700 (PDT) Received: by 10.227.0.140 with HTTP; Tue, 19 Apr 2011 03:55:29 -0700 (PDT) In-Reply-To: References: <4D9D963B.7070001@codesourcery.com> <4DA47B72.7010005@redhat.com> Date: Tue, 19 Apr 2011 11:19:00 -0000 Message-ID: Subject: Re: Fix PR47976 From: Richard Guenther To: Jeff Law Cc: Bernd Schmidt , GCC Patches , Andrew Stubbs , Ramana Radhakrishnan Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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-04/txt/msg01512.txt.bz2 On Tue, Apr 19, 2011 at 11:32 AM, Richard Guenther wrote: > On Tue, Apr 12, 2011 at 6:18 PM, Jeff Law wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> On 04/07/11 04:47, Bernd Schmidt wrote: >>> PR47976 is a followup to PR47166; the patch there caused this problem. >>> >>> The problem occurs in reload. There are two autoinc addresses which >>> inherit from one another, and we delete an insn that is necessary. >>> >>> We reach this code when reloading the second autoinc address: >>> >>> 6821 =A0 =A0 =A0 =A0if (optimize && REG_P (oldequiv) >>> 6822 =A0 =A0 =A0 =A0 =A0 =A0&& REGNO (oldequiv) < FIRST_PSEUDO_REGISTER >>> 6823 =A0 =A0 =A0 =A0 =A0 =A0&& spill_reg_store[REGNO (oldequiv)] >>> 6824 =A0 =A0 =A0 =A0 =A0 =A0&& REG_P (old) >>> (gdb) >>> 6825 =A0 =A0 =A0 =A0 =A0 =A0&& (dead_or_set_p (insn, >>> 6826 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spill_= reg_stored_to[REGNO (oldequiv)]) >>> 6827 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| rtx_equal_p (spill_reg_stored_to= [REGNO (oldequiv)], >>> 6828 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0old= ))) >>> 6829 =A0 =A0 =A0 =A0 =A0delete_output_reload (insn, j, REGNO (oldequiv)= , reloadreg); >>> >>> reload_inherited[j] is 1 at this point, so oldequiv =3D=3D reloadreg. >>> >>> (gdb) p debug_rtx (spill_reg_store[7]) >>> (insn 719 718 232 10 (set (reg:SI 7 r7) >>> =A0 =A0 =A0 =A0 (reg:SI 3 r3 [orig:339 ivtmp.79 ] [339])) -1 (nil)) >>> (gdb) p debug_rtx (spill_reg_stored_to[7]) >>> (reg:SI 3 r3) >>> >>> Prior to the PR47166 patch, we had spill_reg_store[7] equal to insn 718, >>> which doesn't involve register 7 at all: >>> >>> (insn 718 221 719 10 (set (reg:SI 3 r3 [orig:339 ivtmp.79 ] [339]) >>> =A0 =A0 =A0 =A0 (plus:SI (reg:SI 3 r3 [orig:339 ivtmp.79 ] [339]) >>> =A0 =A0 =A0 =A0 =A0 =A0 (const_int 8 [0x8]))) 4 {*arm_addsi3} (nil)) >>> >>> That was sufficient to generate enough confusion to make the compiler >>> think it couldn't delete the output reload. >>> >>> I think the problem is simply that the (set (r7) (r3)) is the opposite >>> direction of a normal spill_reg_store - normally you write a spill reg >>> to its destination, but autoinc reloads are somewhat special. >>> >>> If delete_output_reload isn't valid for (at least some) autoincs, we can >>> simply not record them in spill_reg_store. That's part of the patch >>> below; it seems to fix the problem. I've also deleted the code quoted >>> above since it's pointless to have reload deleting dead stores to >>> registers: that's what DCE is for. I've observed no code generation >>> changes other than for the testcase from either of these changes, with >>> both an ARM and an sh compiler. >>> >>> Comments? >> Looks good to me. =A0I like letting DCE do its job, particularly if it >> allows us to even trivially simplify this code ;-) > > As you are fine with it and a patch deleting more code than it adds > always makes me feel comfortable and as I'm trying to get a 4.5.3 > done which is blocked by this bug I will apply the patch after a round > of testing. > > The bugzilla audit trail says the patch tests fine on a few archs, > I'm going to test x86_64 everywhere and all my available archs > for a 4.5 branch backport. Bootstrapped and tested on x86_64-unknown-linux-gnu for trunk with the set-but-not-used store variable removed. Installed as r172706. I'll wait for some autotester coverage and my own extensive 4.5 testing before doing a 4.6 and 4.5 backport. Richard.