From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Archive: List-Post: List-Help: Sender: gcc-patches-owner@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:08:00 -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 Content-Type: multipart/mixed; boundary="------------040800010402090306060300" X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg01953.txt.bz2 This is a multi-part message in MIME format. --------------040800010402090306060300 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Content-length: 3243 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 elim= ination) 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 v= ersion is trying to change the destination of a defining insn that feeds into an exte= nd insn is not valid if the defining insn doesn't feed directly into the extend ins= n. 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 wou= ld have been to check on that path (when copy_needed in combine_reaching_defs is true) t= hat 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 does= n'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 regr= essions with previous versions of the patch that were less targeted). Bootstrapped and tested on x86_64-unknown-linux-gnu, arm-none-linux-gnueabi= hf, aarch64-none-linux-gnu. I've marked some other PRs that are dups of this one in the ChangeLog. If a= nyone 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. --------------040800010402090306060300 Content-Type: text/x-patch; name=ree-fix.patch Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="ree-fix.patch" Content-length: 2157 commit 33131f774705b936afc1a26c145e1214b388771f Author: Kyrylo Tkachov Date: Fri Nov 13 15:01:47 2015 +0000 [RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in prese= nce 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_p= at, ext_state *state) =20 rtx tmp_reg =3D 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; =20 /* 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 =3D 0; c < 2; c++) + { + d && (f =3D 0); + h =3D fn1 (); + } + __builtin_printf ("%d\n", (char) f); + } + + return 0; +} + +/* { dg-output "0" } */ --------------040800010402090306060300--