From patchwork Wed May 15 13:29:29 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Julian Brown X-Patchwork-Id: 244076 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 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 4B5E72C00A9 for ; Wed, 15 May 2013 23:30:11 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; q=dns; s= default; b=xrdzr9bdQFCuS2SyxW+nQ+skkhcLBjkUROnJB1WMaEOvHB/fqO7gG /Tdcdoiywom9mtMk/KwdsCyyLMCQPYpvTri87+zDvTqyoMyuK+UsWJ6MFhPVCi5t bKxFzEbM1imksZV8cfv1scUc3EMaI7u3+jVZ0VuaLIXC6UH/d0RNTE= 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:date :from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; s=default; bh=V2mXBPFYz02kvwflX/QNhD18B1I=; b=Hg/JpCCjhewWvyeL7+jzSt5FuGzm apN47dcFqOZrxCu/6VCJRGzSe+tgx6WXP21Mq5/DLwNn0RBgW7kFB5RGIe+5yCjH r8DM3uKZvpvk5z0+DxiH3A/yFPrx9xLvk4Pu6mJWdqoIKliQ5mhR1yZn2iy+kc39 2dORzytEmoY9YGo= Received: (qmail 27744 invoked by alias); 15 May 2013 13:29:50 -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 27710 invoked by uid 89); 15 May 2013 13:29:50 -0000 X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_THREADED, RCVD_IN_HOSTKARMA_W, RCVD_IN_HOSTKARMA_WL, TW_JB, TW_RW, TW_WX autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 15 May 2013 13:29:48 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1Ucbm5-0005Gh-OG from Julian_Brown@mentor.com ; Wed, 15 May 2013 06:29:45 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Wed, 15 May 2013 06:29:45 -0700 Received: from octopus (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Wed, 15 May 2013 14:29:42 +0100 Date: Wed, 15 May 2013 14:29:29 +0100 From: Julian Brown To: Julian Brown CC: Jeff Law , , Steven Bosscher Subject: Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) Message-ID: <20130515142929.2c54729b@octopus> In-Reply-To: <20130507160550.0a8f702f@octopus> References: <20130503141019.391ca3f4@octopus> <5187EE10.6010201@redhat.com> <20130507160550.0a8f702f@octopus> MIME-Version: 1.0 On Tue, 7 May 2013 16:05:50 +0100 Julian Brown wrote: > On Mon, 6 May 2013 11:53:20 -0600 > Jeff Law wrote: > > > On 05/03/2013 07:10 AM, Julian Brown wrote: > > > Hi, > > > > > > This is a patch which fixes a latent bug in RTL GCSE/PRE, > > > described more fully in: > > > > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57159 > > > > > > I haven't been able to reproduce the problem on mainline (nor on a > > > supported target). Maybe someone more familiar with the code in > > > question than I am can tell if the patch is correct nonetheless? > > > > Index: gcc/gcse.c > > > =================================================================== > > > --- gcc/gcse.c (revision 198175) > > > +++ gcc/gcse.c (working copy) > > > @@ -3888,6 +3888,13 @@ compute_ld_motion_mems (void) > > > { > > > rtx src = SET_SRC (PATTERN (insn)); > > > rtx dest = SET_DEST (PATTERN (insn)); > > > + rtx note = find_reg_equal_equiv_note (insn); > > > + rtx src_eq; > > > + > > > + if (note != 0 && REG_NOTE_KIND (note) == > > > REG_EQUAL) > > > + src_eq = XEXP (note, 0); > > > + else > > > + src_eq = NULL_RTX; > > > > > > /* Check for a simple LOAD... */ > > > if (MEM_P (src) && simple_mem (src)) > > > @@ -3904,6 +3911,12 @@ compute_ld_motion_mems (void) > > > invalidate_any_buried_refs (src); > > > } > > > > > > + /* Also invalidate any buried loads which may > > > be present in > > > + REG_EQUAL notes. */ > > > + if (src_eq != NULL_RTX > > > + && !(MEM_P (src_eq) && simple_mem > > > (src_eq))) > > > + invalidate_any_buried_refs (src_eq); > > > + > > > /* Check for stores. Don't worry about aliased > > > ones, they will block any movement we might do later. We only care > > > about this exact pattern since those are > > > the only > > What happens if the code contains a simple mem? We don't invalidate > > it as far as I can tell. Doesn't that open us up to the same > > problems that we're seeing with with the non-simple mem? > > I don't know. My assumption was that a "simple" mem would be one that > the PRE pass could handle -- that clause was intended to handle simple > mems in REG_EQUAL notes the same as simple mems as the source of a > set. Maybe that is not safe though, and it would be better to > unconditionally invalidate buried mems in REG_EQUAL notes? I was > slightly wary of inhibiting genuine optimisation opportunities. A not-very-scientific data point concerning the last part: I tried a patch which invalidated buried refs in any REG_EQUAL note, i.e.: Running a bootstrap for x86_64, this gives a cc1 binary: -rwxr-xr-x 1 jbrown eeg 123362809 2013-05-15 03:24 cc1 Without the patch, the binary is slightly smaller: -rwxr-xr-x 1 jbrown eeg 123362481 2013-05-15 02:45 cc1 With the previously-posted patch which does not invalidate buried refs for simple mems, I get: -rwxr-xr-x 1 jbrown eeg 123362377 2013-05-15 04:08 cc1 i.e. slightly *smaller* than the baseline. I haven't investigated more deeply than that, but that seems to be a tiny bit of evidence at least that unconditionally invalidating buried refs (as above) might not be a good idea. There are no testsuite regressions with the above patch (for gcc/g++/libstdc++), FWIW. Cheers, Julian --- gcc/gcse.c (revision 198880) +++ gcc/gcse.c (working copy) @@ -3894,6 +3894,7 @@ compute_ld_motion_mems (void) { rtx src = SET_SRC (PATTERN (insn)); rtx dest = SET_DEST (PATTERN (insn)); + rtx note = find_reg_equal_equiv_note (insn); /* Check for a simple LOAD... */ if (MEM_P (src) && simple_mem (src)) @@ -3910,6 +3911,11 @@ compute_ld_motion_mems (void) invalidate_any_buried_refs (src); } + /* Also invalidate any buried loads which may be present in + REG_EQUAL notes. */ + if (note != NULL_RTX && REG_NOTE_KIND (note) == REG_EQUAL) + invalidate_any_buried_refs (XEXP (note, 0)); + /* Check for stores. Don't worry about aliased ones, they will block any movement we might do later. We only care about this exact pattern since those are the only