From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15589 invoked by alias); 13 Jan 2015 22:17:16 -0000 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 Received: (qmail 15502 invoked by uid 89); 13 Jan 2015 22:17:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 13 Jan 2015 22:17:13 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0DMHALG018278 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 13 Jan 2015 17:17:10 -0500 Received: from [10.3.113.77] (ovpn-113-77.phx2.redhat.com [10.3.113.77]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t0DMH9vx006485; Tue, 13 Jan 2015 17:17:09 -0500 Message-ID: <54B59964.7070707@redhat.com> Date: Tue, 13 Jan 2015 22:28:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Jakub Jelinek CC: Segher Boessenkool , Richard Biener , Eric Botcazou , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Reenable CSE of non-volatile inline asm (PR rtl-optimization/63637) References: <20150113161819.GD1405@tucnak.redhat.com> <20150113163840.GA4183@gate.crashing.org> <54B575D7.8030107@redhat.com> <20150113201322.GJ1405@tucnak.redhat.com> In-Reply-To: <20150113201322.GJ1405@tucnak.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-01/txt/msg00949.txt.bz2 On 01/13/15 13:13, Jakub Jelinek wrote: > On Tue, Jan 13, 2015 at 12:45:27PM -0700, Jeff Law wrote: >> On 01/13/15 09:38, Segher Boessenkool wrote: >>> On Tue, Jan 13, 2015 at 05:18:19PM +0100, Jakub Jelinek wrote: >>>> 3) on request from Richard (which Segher on IRC argues against), "memory" >>>> clobber also prevents CSE; >>> >>> As extend.texi used to say: >>> >>> " >>> If your assembler instructions access memory in an unpredictable >>> fashion, add @samp{memory} to the list of clobbered registers. This >>> causes GCC to not keep memory values cached in registers across the >>> assembler instruction and not optimize stores or loads to that memory. >>> You also should add the @code{volatile} keyword if the memory >>> affected is not listed in the inputs or outputs of the @code{asm}, as >>> the @samp{memory} clobber does not count as a side-effect of the >>> @code{asm}. >>> " >>> >>> so a "memory" clobber in a non-volatile asm should not prevent CSE. >> My reading of that paragraph is somewhat different. >> >> The key here is the memory clobber affects optimization of instructions >> around the asm while the volatile specifier affects the optimization of the >> ASM itself. >> >> A memory clobber must inhibit CSE of memory references on either side of the >> asm because the asm must be assumed to read or write memory in unpredictable >> ways. >> >> The volatile specifier tells the compiler that the asm itself must be >> preserved, even if dataflow shows the outputs as not used. > > That is not necessarily in conflict. Possibly not :-) This stuff isn't trivial and as well meaning as folks trying to update the docs have been, it's possible subtle issues like these have been missed, even in the review process. I know that for me it's easier to reason about code changes like this than their associated documentation :-) > My reading of Jeff's comment is that in > int a; > int > foo (void) > { > int b, c, d, e; > b = a; > asm ("..." : "=r" (c) : : "memory"); > d = a; > asm ("..." : "=r" (e) : : "memory"); > return b + d + 2 * (c + e); > } > we are not allowed to CSE d = a; into d = b; Precisely. At least that's how I read things and it makes sense to the part of my brain that used to split time between kernel & GCC development in a previous life. In effect the "memory" clobber is an aggregation of the read, write, clobbers dataflow for memory (and imprecise as it hits all memory). . CSE invalidate_from_clobbers > should ensure that already, even when we don't do anything special about > "memory" clobber in the patch. OK. Another thing is if there is a store > in between the two non-volatile asms with "memory" clobber, here I'm not > sure if with the alternate patch we'd treat the "memory" clobber as use of > everything previously stored into memory (in this regard the posted version > is safe). I woudln't be terribly surprised if DSE isn't safe in this regard. I don't recall CSE doing any kind of dead store elimination so it wouldn't likely care that the memory clobber implies a read as well. > And finally there is the case of non-volatile asm with "memory" clobber with > no memory stores in between the two - the posted (safer) patch will not > allow to CSE the two, while in theory we could CSE them into just one asm. I think we have to assume that CSEing them is wrong. The first may set something in memory that is read by the second. Thoughts? Jeff