From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21544 invoked by alias); 14 Jan 2015 06:42:47 -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 21531 invoked by uid 89); 14 Jan 2015 06:42:46 -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,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; Wed, 14 Jan 2015 06:42:45 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0E6gdBQ025697 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 14 Jan 2015 01:42:39 -0500 Received: from [10.3.113.77] (ovpn-113-77.phx2.redhat.com [10.3.113.77]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t0E6gcFe017590; Wed, 14 Jan 2015 01:42:38 -0500 Message-ID: <54B60FDE.4030902@redhat.com> Date: Wed, 14 Jan 2015 07:12: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 , Richard Biener , Eric Botcazou CC: gcc-patches@gcc.gnu.org, Segher Boessenkool Subject: Re: [PATCH] Reenable CSE of non-volatile inline asm (PR rtl-optimization/63637) References: <20150113161819.GD1405@tucnak.redhat.com> In-Reply-To: <20150113161819.GD1405@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/msg00973.txt.bz2 On 01/13/15 09:18, Jakub Jelinek wrote: > Hi! > > My PR60663 fix unfortunately stopped CSE of all inline-asms, even when > they e.g. only have the clobbers added by default. > > This patch attempts to restore the old behavior, with the exceptions: > 1) as always, asm volatile is not CSEd > 2) inline-asm with multiple outputs are not CSEd > 3) on request from Richard (which Segher on IRC argues against), "memory" > clobber also prevents CSE; this can be removed by removing the > int j, lim = XVECLEN (x, 0); and loop below it > 4) inline-asm with clobbers is never copied into an insn that wasn't > inline-asm before, so if there are clobbers, we allow CSEing of > e.g. two same inline-asms, but only by reusing results of one > of those > > Bootstrapped/regtested on x86_64-linux and i686-linux, tested also > with arm cross after reverting the PR60663 arm cost fix. > > Ok for trunk this way, or with 3) removed? > > 2015-01-13 Jakub Jelinek > > PR rtl-optimization/63637 > PR rtl-optimization/60663 > * cse.c (merge_equiv_classes): Set new_elt->cost to MAX_COST > if elt->cost is MAX_COST for ASM_OPERANDS. > (find_sets_in_insn): Fix up comment typo. > (cse_insn): Don't set src_volatile for all non-volatile > ASM_OPERANDS in PARALLELs, but just those with multiple outputs > or with "memory" clobber. Set elt->cost to MAX_COST > for ASM_OPERANDS in PARALLEL. Set src_elt->cost to MAX_COST > if new_src is ASM_OPERANDS and elt->cost is MAX_COST. > > * gcc.dg/pr63637-1.c: New test. > * gcc.dg/pr63637-2.c: New test. > * gcc.dg/pr63637-3.c: New test. > * gcc.dg/pr63637-4.c: New test. > * gcc.dg/pr63637-5.c: New test. > * gcc.dg/pr63637-6.c: New test. > * gcc.target/i386/pr63637-1.c: New test. > * gcc.target/i386/pr63637-2.c: New test. > * gcc.target/i386/pr63637-3.c: New test. > * gcc.target/i386/pr63637-4.c: New test. > * gcc.target/i386/pr63637-5.c: New test. > * gcc.target/i386/pr63637-6.c: New test. > > --- gcc/cse.c.jj 2015-01-09 21:59:44.000000000 +0100 > +++ gcc/cse.c 2015-01-13 13:26:23.391216064 +0100 > @@ -1792,6 +1792,8 @@ merge_equiv_classes (struct table_elt *c > } > new_elt = insert (exp, class1, hash, mode); > new_elt->in_memory = hash_arg_in_memory; > + if (GET_CODE (exp) == ASM_OPERANDS && elt->cost == MAX_COST) > + new_elt->cost = MAX_COST; > } > } > } > @@ -4258,7 +4260,7 @@ find_sets_in_insn (rtx_insn *insn, struc > { > int i, lim = XVECLEN (x, 0); > > - /* Go over the epressions of the PARALLEL in forward order, to > + /* Go over the expressions of the PARALLEL in forward order, to > put them in the same order in the SETS array. */ > for (i = 0; i < lim; i++) > { > @@ -4634,12 +4636,27 @@ cse_insn (rtx_insn *insn) > && REGNO (dest) >= FIRST_PSEUDO_REGISTER) > sets[i].src_volatile = 1; > > - /* Also do not record result of a non-volatile inline asm with > - more than one result or with clobbers, we do not want CSE to > - break the inline asm apart. */ > else if (GET_CODE (src) == ASM_OPERANDS > && GET_CODE (x) == PARALLEL) > - sets[i].src_volatile = 1; > + { > + /* Do not record result of a non-volatile inline asm with > + more than one result. */ > + if (n_sets > 1) > + sets[i].src_volatile = 1; > + > + int j, lim = XVECLEN (x, 0); > + for (j = 0; j < lim; j++) > + { > + rtx y = XVECEXP (x, 0, j); > + /* And do not record result of a non-volatile inline asm > + with "memory" clobber. */ > + if (GET_CODE (y) == CLOBBER && MEM_P (XEXP (y, 0))) Can you please add a comment here which references the full form of the "memory" tag. (clobber (mem:BLK (scratch))). If we ever have to look at this again (say perhaps to break out the read anything vs write anything into separate tags :-) it'll save considerable time and angst trying to track all this stuff down. The tests you've got are a step forward, but there's obviously a lot more we could do. For example testing DSE around ASMs without and without a memory "clobber", testing CSE of unrelated memory references around an ASM without and without a memory clobber come to mind. You don't have to add them to get approval, but if you were to take the time to cobble them together it'd be hugely appreciated. Given the discussion with Segher, let's give him a chance to chime in on tonight's messages before we make a final decision. jeff