From patchwork Mon Nov 16 14:07:47 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 56644 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp1336368lbb; Mon, 16 Nov 2015 06:08:14 -0800 (PST) X-Received: by 10.66.254.73 with SMTP id ag9mr54470535pad.116.1447682893960; Mon, 16 Nov 2015 06:08:13 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id xm2si50905057pbb.66.2015.11.16.06.08.13 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Nov 2015 06:08:13 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-414237-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; spf=pass (google.com: domain of gcc-patches-return-414237-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-414237-patch=linaro.org@gcc.gnu.org; dkim=pass header.i=@gcc.gnu.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:content-type; q=dns; s=default; b=bYXW1WRKnm1OddtPMRnAgKFKei9O5+5vd4Uhz2OeHXU lZtpPsUbsmfDhOLjHFSRBFEic3eN0GdqAQs1j1RuIf+12EqvYtbKcgTd1FoSXKds OdtiISJ3NWRedV5YqpsOtIK6R4wrcY8phhE3P3f2I1N/Ar2gSkTibOGWmKSlC0wI = 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 :message-id:date:from:mime-version:to:cc:subject:content-type; s=default; bh=NJ85ksjkuy0QZyG9olINcqAXSjQ=; b=oIKdj4eWVjxaX6FQt E11fN1LIf2eVDSX1EPAt3L4keyuThLSv3Gy6n01+S2h4ZsglSQIo6bF/vwRDvLcz 8yvQ7aSlvgkQftvINe9JRO7dAKFr2QJOBgMMqcXmsKAT9tt9PaN0whbaqE9syFY8 AuTWTMTBhbS76Wn31vmwB3PLPc= Received: (qmail 65459 invoked by alias); 16 Nov 2015 14:08:00 -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 64661 invoked by uid 89); 16 Nov 2015 14:07:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (207.82.80.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 16 Nov 2015 14:07:57 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-26-lAXUHDpATaG1XFXrfvxX6w-1; Mon, 16 Nov 2015 14:07:47 +0000 Received: from [10.2.206.200] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 16 Nov 2015 14:07:47 +0000 Message-ID: <5649E333.4090904@arm.com> Date: Mon, 16 Nov 2015 14:07:47 +0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: GCC Patches CC: Jeff Law Subject: [PATCH][RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves X-MC-Unique: lAXUHDpATaG1XFXrfvxX6w-1 X-IsSubscribed: yes Hi all, In this PR we encounter a wrong-code bug on x86_64. It started with an RTL if-conversion patch of mine which I believe exposed a latent issue in the ree (redundant extension elimination) pass. The different if-conversion behaviour enabled a new cse opportunity which then produced the RTL that triggered the bad ree behaviour. The relevant RTL insns before the ree pass look like: Basic Block A: (set (reg:HI 0 ax) (mem:HI (symbol_ref:DI ("f")))) ... (set (reg:SI 3 bx) (if_then_else:SI (eq (reg:CCZ 17 flags) (const_int 0)) (reg:SI 0 ax) (reg:SI 3 bx))) (set (reg:SI 4 si) (sign_extend:SI (reg:HI 3 bx))) ... Basic block B (dominated by basic block A): (set (reg:SI 4 si) (sign_extend:SI (reg:QI 0 ax))) /* ax contains symbol "f". */ ree changes that into the broken: Basic block A: (set (reg:SI 4 si) (sign_extend:SI (mem:HI (symbol_ref:DI ("f"))))) (set (reg:SI 3 bx) (reg:SI 4 si)) ... (set (reg:SI 3 bx) (if_then_else:SI (eq (reg:CCZ 17 flags) (const_int 0 [0])) (reg:SI 0 ax) (reg:SI 3 bx))) ... Basic block B: (set (reg:SI 4 si) (sign_extend:SI (reg:QI 0 ax))) /* Insn unchanged by ree, but ax now undefined. */ Note that after ree register ax is now used uninitialised in basic block A and the insn that previously set it to symbol "f" has now been transformed into: (set (reg:SI 4 si) (sign_extend:SI (mem:HI (symbol_ref:DI ("f"))))) I've explained in the comments in the patch what's going on but the short version is trying to change the destination of a defining insn that feeds into an extend insn is not valid if the defining insn doesn't feed directly into the extend insn. In the ree pass the only way this can happen is if there is an intermediate conditional move that the pass tries to handle in a special way. An equivalent fix would have been to check on that path (when copy_needed in combine_reaching_defs is true) that the state->copies_list vector (that contains the conditional move insns feeding into the extend insn) is empty. This patch is the minimal fix that I could do for this PR and the cases it rejects are cases where we're pretty much guaranteed to miscompile the code, so it doesn't reject any legitimate extension elimination opportunities. I checked that the code generation doesn't change on aarch64 for the whole of SPEC2006 (I did see codegen regressions with previous versions of the patch that were less targeted). Bootstrapped and tested on x86_64-unknown-linux-gnu, arm-none-linux-gnueabihf, aarch64-none-linux-gnu. I've marked some other PRs that are dups of this one in the ChangeLog. If anyone has any other PRs that are dups of this one please let me know. Ok for trunk? Thanks, Kyrill 2015-11-16 Kyrylo Tkachov PR rtl-optimization/68194 PR rtl-optimization/68328 PR rtl-optimization/68185 * ree.c (combine_reaching_defs): Reject copy_needed case if defining insn does not feed directly into candidate extension insn. 2015-11-16 Kyrylo Tkachov PR rtl-optimization/68194 * gcc.dg/pr68194.c: New test. commit 33131f774705b936afc1a26c145e1214b388771f Author: Kyrylo Tkachov Date: Fri Nov 13 15:01:47 2015 +0000 [RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves diff --git a/gcc/ree.c b/gcc/ree.c index b8436f2..e91d164 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -814,7 +814,30 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) rtx tmp_reg = gen_rtx_REG (GET_MODE (SET_DEST (PATTERN (cand->insn))), REGNO (SET_DEST (*dest_sub_rtx))); - if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn)))) + + /* When transforming: + (set (reg1) (expression)) + ... + (set (reg2) (any_extend (reg1))) + + into + + (set (reg2) (any_extend (expression))) + (set (reg1) (reg2)) + make sure that reg1 from the first set feeds directly into the extend. + This may not hold in a situation with an intermediate + conditional copy i.e. + I1: (set (reg3) (expression)) + I2: (set (reg1) (cond ? reg3 : reg1)) + I3: (set (reg2) (any_extend (reg1))) + + where I3 is cand, I1 is def_insn and I2 is a conditional copy. + We want to avoid transforming that into: + (set (reg2) (any_extend (expression))) + (set (reg1) (reg2)) + (set (reg1) (cond ? reg3 : reg1)). */ + if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn))) + || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn)))) return false; /* The destination register of the extension insn must not be diff --git a/gcc/testsuite/gcc.dg/pr68194.c b/gcc/testsuite/gcc.dg/pr68194.c new file mode 100644 index 0000000..b4855ea --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr68194.c @@ -0,0 +1,35 @@ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +int a, c, d, e, g, h; +short f; + +short +fn1 (void) +{ + int j[2]; + for (; e; e++) + if (j[0]) + for (;;) + ; + if (!g) + return f; +} + +int +main (void) +{ + for (; a < 1; a++) + { + for (c = 0; c < 2; c++) + { + d && (f = 0); + h = fn1 (); + } + __builtin_printf ("%d\n", (char) f); + } + + return 0; +} + +/* { dg-output "0" } */