From patchwork Fri Sep 10 11:57:28 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans-Peter Nilsson X-Patchwork-Id: 64383 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 910E7B711E for ; Fri, 10 Sep 2010 21:57:45 +1000 (EST) Received: (qmail 27891 invoked by alias); 10 Sep 2010 11:57:40 -0000 Received: (qmail 27868 invoked by uid 22791); 10 Sep 2010 11:57:38 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL, BAYES_00, TW_FC, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from ra.se.axis.com (HELO ra.se.axis.com) (195.60.68.13) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 10 Sep 2010 11:57:32 +0000 Received: from localhost (localhost [127.0.0.1]) by ra.se.axis.com (Postfix) with ESMTP id E898311E92 for ; Fri, 10 Sep 2010 13:57:29 +0200 (CEST) Received: from ra.se.axis.com ([127.0.0.1]) by localhost (ra.se.axis.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id b1rIFPhHLG1M for ; Fri, 10 Sep 2010 13:57:29 +0200 (CEST) Received: from miranda.se.axis.com (miranda.se.axis.com [10.0.2.171]) by ra.se.axis.com (Postfix) with ESMTPS id 1CF8D11E88 for ; Fri, 10 Sep 2010 13:57:29 +0200 (CEST) Received: from ignucius.se.axis.com (ignucius.se.axis.com [10.88.21.50]) by miranda.se.axis.com (8.13.4/8.13.4/Debian-3sarge3) with ESMTP id o8ABvSPt004862; Fri, 10 Sep 2010 13:57:28 +0200 Received: from ignucius.se.axis.com (localhost [127.0.0.1]) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) with ESMTP id o8ABvSF6016250; Fri, 10 Sep 2010 13:57:28 +0200 Received: (from hp@localhost) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) id o8ABvSFh016246; Fri, 10 Sep 2010 13:57:28 +0200 Date: Fri, 10 Sep 2010 13:57:28 +0200 Message-Id: <201009101157.o8ABvSFh016246@ignucius.se.axis.com> From: Hans-Peter Nilsson To: gcc-patches@gcc.gnu.org Subject: Fix PR41087, ifcvt breakage for condition with postincrement MIME-Version: 1.0 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 This is a regression since 4.5; latent since forever, exposed by autocrement improvements. In the case at hand, there was an apparent trivial store_flag elimination opportunity (setting a variable to 0 in the then-arm and to 1 in the else-arm), but with a twist: the condition contained a (mem/s:SI (post_inc:SI (reg:SI 241 [ ivtmp.23 ])) [3 S4 A8]) so when the full store-flag sequence was emitted, *including a copy of the compare insn*, the compare insn "stuck" (was not removed as dead by later passes), supposedly because it had a side-effect. My first inclination was that ifcvt should remove the compare insn for the replaced conditional branch, but apparently ifcvt is designed to emit the new sequence just before the old conditional jump (after old compare insns), relying on later passes to remove the (redundant) compare insn. This *should* work because the responsible function, noce_get_condition, is supposed to be checking that "the resulting condition must be valid at JUMP" (like, copies of it will be benign). Which it isn't when the condition has side-effects. Besides, ifcvt doesn't decorate the new compare insn with the necessary REG_INC note that the next pass, regmove, expects. One *could* fix this bug by forcing the offending part of the condition to a register (pre-register-allocation), but that's a bit too involved (e.g. it should be undone if the if-statement after all can't be optimized but the insn-sequence would have to be kept valid for other parts of ifcvt to work, so the register can't be trivially substituted just in the insn but has to be fully emitted as a move sequence). Anyway, despite that the failing test-case, gfortran.dg/zero_sized_3.f90 contained lots of ifcvt-like conditional assignments after expansion and loop unrolling, this miscompiled case was the only one trigging in ifcvt at all for (not just post-combine). Most simple ifcvt cases seem already taken care of at expand time. Point being, an involved kind of solution doesn't seem worthwhile. Better to just make noce_get_condition do what it says. As follows. See also PR11052, where a similar thing happened to the target (the x in "if (c) x = a; else x = b;"). Regtested native and cross to cris-axis-elf (trunk; 4.5 in progress). Ok for trunk and 4.5? gcc: PR rtl-optimization/41087 * ifcvt.c (noce_get_condition): Don't allow conditions with side-effects. brgds, H-P Index: ifcvt.c =================================================================== --- ifcvt.c (revision 164149) +++ ifcvt.c (working copy) @@ -2201,8 +2201,15 @@ noce_get_condition (rtx jump, rtx *earli /* Otherwise, fall back on canonicalize_condition to do the dirty work of manipulating MODE_CC values and COMPARE rtx codes. */ - return canonicalize_condition (jump, cond, reverse, earliest, - NULL_RTX, false, true); + tmp = canonicalize_condition (jump, cond, reverse, earliest, + NULL_RTX, false, true); + + /* We don't handle side-effects in the condition, like handling + REG_INC notes and making sure no duplicate conditions are emitted. */ + if (tmp != NULL_RTX && side_effects_p (tmp)) + return NULL_RTX; + + return tmp; } /* Return true if OP is ok for if-then-else processing. */