From patchwork Thu Oct 18 15:21:02 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Teresa Johnson X-Patchwork-Id: 192352 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]) by ozlabs.org (Postfix) with SMTP id 1A9C52C008E for ; Fri, 19 Oct 2012 02:21:41 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1351178502; h=Comment: DomainKey-Signature:Received:Received:Received:Received: MIME-Version:Received:Received:In-Reply-To:References:Date: Message-ID:Subject:From:To:Cc:Content-Type:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=mGx/Yukm7umocvTRKIzSkdbJ7cg=; b=msgfTakdFn3lKPQ+uZptd5RjeV2//ybonwpVyMahYuF2HY13EqAiiAhWw/zBBQ 907gdlVA8kdPr/0eMDr9JWN34QRqyunSuV3FwC0qJMlwHd4+Q9zg5KN+7OLMyhap WNNTxbHF9t2sVuzzGXqDJOaiTyIf4Dl1WiScV8QKEfJ4s= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Google-DKIM-Signature:MIME-Version:Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Cc:Content-Type:X-System-Of-Record:X-Gm-Message-State:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=siAdtHnoYLYkLrvLV4XNly1NB4w8ORLgK8Pv1YCwWA+TGUZZLuwwdn8XKefwdJ X5OUB4dGwt1M2zmxn/Optm+Prx+LIfbaLO1+cC3Fq+y31a0RY8pgIBuWMPQRtve4 2nf0SwpRUfXpzj7IfqxHw/NXRVSLuhR5ephkNzvGiynms=; Received: (qmail 30311 invoked by alias); 18 Oct 2012 15:21:11 -0000 Received: (qmail 30288 invoked by uid 22791); 18 Oct 2012 15:21:10 -0000 X-SWARE-Spam-Status: No, hits=-5.6 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, RP_MATCHES_RCVD, TW_SB, TW_VZ, TW_ZB X-Spam-Check-By: sourceware.org Received: from mail-pb0-f47.google.com (HELO mail-pb0-f47.google.com) (209.85.160.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 18 Oct 2012 15:21:03 +0000 Received: by mail-pb0-f47.google.com with SMTP id ro12so8567938pbb.20 for ; Thu, 18 Oct 2012 08:21:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record:x-gm-message-state; bh=NcgtVbIvkoSc5GsBanQ3/DVXUDEdhhuFLgQfk72JdbY=; b=M3gsI9lCh0ci0/IqyboxfTUMI2tGgfsH0Te7f6ECraiTjEzuRESNJdKqETIIIsS5Nw PzCbMMz5WFnL3Z3tpkHxMA7e8OwAH/AtS5x9PzRNXW30yv41Cv1Uw7k4INZLdCEXLrVH 1lDY31k1ZvV+/j8L5AeKaeZ39J0AGvK08MMVFTcJ8PbmIj5B6GYJpkRLtaa626Js+fXY hlFJu6YxVR6zzKjDtq9G757CtR4ydqF22PwFLo+U/sVvUWD2IuVlbHuAVHHW2451gKtG Amx2NspeN91hYm89S4WoImrKhIwA3PVuhZD3jaA2fV3n4+R3A7T9z8irsgrpG4QIn+Kq annQ== MIME-Version: 1.0 Received: by 10.68.252.133 with SMTP id zs5mr66805730pbc.152.1350573662548; Thu, 18 Oct 2012 08:21:02 -0700 (PDT) Received: by 10.68.189.38 with HTTP; Thu, 18 Oct 2012 08:21:02 -0700 (PDT) In-Reply-To: References: <20121011214412.5AC876128C@tjsboxrox.mtv.corp.google.com> <20121012082353.GK584@tucnak.redhat.com> Date: Thu, 18 Oct 2012 08:21:02 -0700 Message-ID: Subject: Re: [PATCH] Reduce conservativeness in REE using machine model (issue6631066) From: Teresa Johnson To: Jakub Jelinek Cc: reply@codereview.appspotmail.com, davidxl@google.com, gcc-patches@gcc.gnu.org X-System-Of-Record: true X-Gm-Message-State: ALoCoQllvh5+rwLomvYRAj6D55lT4uA5KHQhWf8dtc8Xd8AEtqlQRNHsU1PR7HSNrwi8sx9Ui+dvB/aZ0xyo5HK2MinQMK9TynN0GnMiEIvJd6yNSACfzaKzfarien5gBXK0bO0Ifr4Jo4mCFCO1oakX1bBL7oHNYqcG7xO3q+tWN34oQkx8O8KHn3Qh2wbob27N5FHUOyHN X-IsSubscribed: yes 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 The attached patch implements avoids conservative behavior in REE by allowing removal of redundant extends when the def feeds another extend with a different mode. This works because in merge_def_and_ext only calls combine_set_extension if the candidate for removal has a wider mode than the def extend's mode, otherwise the def extend mode is preserved. In combine_set_extension the def is modified to use the wider candidate's mode. See my previous message in this thread for more description of why this solution is preferred and works. Added two test cases to check for removal of redundant zero and sign extends. Bootstrapped and tested on x86_64-unknown-linux-gnu. Is this ok for trunk? Thanks, Teresa 2012-10-18 Teresa Johnson * ree.c (add_removable_extension): Remove unnecessary mode check with other extension. 2012-10-18 Teresa Johnson * gcc.c-torture/execute/20111227-2.c: New test. * gcc.c-torture/execute/20111227-3.c: Ditto. Index: gcc.c-torture/execute/20111227-2.c =================================================================== /* Testcase derived from 20111227-1.c to ensure that REE is combining redundant zero extends with zero extend to wider mode. */ /* { dg-options "-fdump-rtl-ree -O" } */ extern void abort (void); unsigned short s; unsigned int i; unsigned long l; unsigned char v = -1; void __attribute__((noinline,noclone)) bar (int t) { if (t == 2 && s != 0xff) abort (); if (t == 1 && i != 0xff) abort (); if (t == 0 && l != 0xff) abort (); } void __attribute__((noinline,noclone)) foo (unsigned char *a, int t) { unsigned char r = v; if (t == 2) s = (unsigned short) r; else if (t == 1) i = (unsigned int) r; else if (t == 0) l = (unsigned long) r; bar (t); } int main(void) { foo (&v, 0); foo (&v, 1); foo (&v, 2); return 0; } /* { dg-final { scan-rtl-dump "Elimination opportunities = 3 realized = 3" "ree" } } */ /* { dg-final { cleanup-rtl-dump "ree" } } */ Index: gcc.c-torture/execute/20111227-3.c =================================================================== /* Testcase derived from 20111227-1.c to ensure that REE is combining redundant sign extends with sign extend to wider mode. */ /* { dg-options "-fdump-rtl-ree -O" } */ extern void abort (void); signed short s; signed int i; signed long l; signed char v = -1; void __attribute__((noinline,noclone)) bar (int t) { if (t == 2 && s != -1) abort (); if (t == 1 && i != -1) abort (); if (t == 0 && l != -1) abort (); } void __attribute__((noinline,noclone)) foo (signed char *a, int t) { signed char r = v; if (t == 2) s = (signed short) r; else if (t == 1) i = (signed int) r; else if (t == 0) l = (signed long) r; bar (t); } int main(void) { foo (&v, 0); foo (&v, 1); foo (&v, 2); return 0; } /* { dg-final { scan-rtl-dump "Elimination opportunities = 3 realized = 3" "ree" } } */ /* { dg-final { cleanup-rtl-dump "ree" } } */ On Mon, Oct 15, 2012 at 2:04 PM, Teresa Johnson wrote: > On Fri, Oct 12, 2012 at 1:23 AM, Jakub Jelinek wrote: >> On Thu, Oct 11, 2012 at 02:44:12PM -0700, Teresa Johnson wrote: >>> Revised patch to address conservative behavior in redundant extend >>> elimination that was resulting in redundant extends not being >>> removed. Now uses a new target hook machine_mode_from_attr_mode >>> which is currently enabled only for i386. >> >> I still don't like it, the hook still is about how it is implemented >> instead of what target property it wants to ask (the important thing >> there is that a {QI,HI} -> SImode zero extension instruction on x86_64 >> performs {QI,HI} -> DImode extension actually). That isn't the case for any >> other modes, isn't the case for sign extension etc. > > That's true, although for sign extends the attr modes being set in > i386.md ensure that this won't do the wrong thing as the the attr > modes in the machine desc file match the machine_mode. However, this > ends up leading to the conservative behavior remaining for sign > extends (see testcase below). > >> >> Can you please post a testcase first? > > This was exposed by the snappy decompression code. However, I was able > to reproduce it by modifying the testcase submitted with the fix that > introduced this checking (gcc.c-torture/execute/20111227-1.c for > http://gcc.gnu.org/ml/gcc-patches/2011-12/msg01744.html). This test > case was failing because a sign_extend was being combined with a > zero_extend, so the instruction code check below fixed it, and the > mode check was unnecessary: > > /* Second, make sure the reaching definitions don't feed another and > different extension. FIXME: this obviously can be improved. */ > for (def = defs; def; def = def->next) > if ((idx = def_map[INSN_UID(DF_REF_INSN (def->ref))]) > && (cand = &VEC_index (ext_cand, *insn_list, idx - 1)) > && (cand->code != code || cand->mode != mode)) > > Here is my modified test case that exposes the conservative behavior I > saw in snappy: > > ------------------------ > extern void abort (void); > > unsigned short s; > unsigned int i; > unsigned long l; > unsigned char v = -1; > > void __attribute__((noinline,noclone)) > bar (int t) > { > if (t == 2 && s != 0xff) > abort (); > if (t == 1 && i != 0xff) > abort (); > if (t == 0 && l != 0xff) > abort (); > } > > void __attribute__((noinline,noclone)) > foo (unsigned char *a, int t) > { > unsigned char r = v; > > if (t == 2) > s = (unsigned short) r; > else if (t == 1) > i = (unsigned int) r; > else if (t == 0) > l = (unsigned long) r; > bar (t); > } > > int main(void) > { > foo (&v, 0); > foo (&v, 1); > foo (&v, 2); > return 0; > } > ----------------------- > > With trunk, there are currently 3 movzbl generated for foo(): > > movzbl v(%rip), %eax > movzbl %al, %eax > movzbl %al, %eax > > With my fix this goes down to 1 movzbl. However, if the test case is > modified to use signed instead of unsigned, we still end up with 3 > movsbl, of which 2 are redundant: > > movsbw v(%rip), %ax > movsbq %al, %rax > movsbl %al, %eax > > A single movsbq will suffice. But because of the attr mode settings > for sign extends I mentioned above, my patch does not help here. > >> >> Given the recent ree.c changes to remember the performed operations and >> their original modes (struct ext_modified), perhaps the >> "Second, make sure the reaching definitions don't feed another and"... >> check could be made less strict or even removed, but for that a testcase is >> really needed. > > I believe that we can remove the mode check from the above code > altogether. The reason is that the ree.c code will always select the > widest mode when combining extends (in merge_def_and_ext). So with a > simple change to the ree.c code to simply avoid the mode checking both > the unsigned and signed cases get addressed. In the signed case we are > left with a single movs: > > movsbq v(%rip), %rax > > I didn't see any test failures in a x86_64-unknown-linux-gnu bootstrap > and regression test run. If this seems reasonable, I can follow up > with the patch (which is trivial), and I can also submit the 2 new > test cases (the signed test case is included below). > > Thanks, > Teresa > > > extern void abort (void); > > signed short s; > signed int i; > signed long l; > signed char v = -1; > > void __attribute__((noinline,noclone)) > bar (int t) > { > if (t == 2 && s != -1) > abort (); > if (t == 1 && i != -1) > abort (); > if (t == 0 && l != -1) > abort (); > } > > void __attribute__((noinline,noclone)) > foo (signed char *a, int t) > { > signed char r = v; > > if (t == 2) > s = (signed short) r; > else if (t == 1) > i = (signed int) r; > else if (t == 0) > l = (signed long) r; > bar (t); > } > > int main(void) > { > foo (&v, 0); > foo (&v, 1); > foo (&v, 2); > return 0; > } > > > >> >> Jakub > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 Index: ree.c =================================================================== --- ree.c (revision 192265) +++ ree.c (working copy) @@ -803,7 +803,7 @@ add_removable_extension (const_rtx expr, rtx insn, for (def = defs; def; def = def->next) if ((idx = def_map[INSN_UID(DF_REF_INSN (def->ref))]) && (cand = &VEC_index (ext_cand, *insn_list, idx - 1)) - && (cand->code != code || cand->mode != mode)) + && cand->code != code) { if (dump_file) {