From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99050 invoked by alias); 9 Oct 2015 09:35:15 -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 99037 invoked by uid 89); 9 Oct 2015 09:35:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-yk0-f174.google.com Received: from mail-yk0-f174.google.com (HELO mail-yk0-f174.google.com) (209.85.160.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 09 Oct 2015 09:35:13 +0000 Received: by ykdg206 with SMTP id g206so73155182ykd.1 for ; Fri, 09 Oct 2015 02:35:11 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.129.125.6 with SMTP id y6mr8465796ywc.5.1444383311048; Fri, 09 Oct 2015 02:35:11 -0700 (PDT) Received: by 10.37.93.136 with HTTP; Fri, 9 Oct 2015 02:35:11 -0700 (PDT) In-Reply-To: References: <20150723203112.GB27818@gate.crashing.org> <20150810082355.GA31149@arm.com> <55C8BFC3.3030603@redhat.com> <55E72D4C.40705@arm.com> <55FC3171.7040509@arm.com> Date: Fri, 09 Oct 2015 09:35:00 -0000 Message-ID: Subject: Re: [PR67828] don't unswitch loops on undefined SSA values (was: Re: [PR64164] drop copyrename, integrate into expand) From: Richard Biener To: Alexandre Oliva Cc: Zhendong Su , Alan Lawrence , Jeff Law , James Greenhalgh , "H.J. Lu" , Segher Boessenkool , GCC Patches , Christophe Lyon , David Edelsohn , Eric Botcazou Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00942.txt.bz2 On Fri, Oct 9, 2015 at 7:26 AM, Alexandre Oliva wrote: > This patch fixes a latent bug in loop unswitching exposed by the PR64164 > changes. > > We would move a test out of a loop that might never have been executed, > and that accessed an uninitialized variable. The uninitialized SSA > name, due to uncprop, now gets coalescesd with other SSA names, > expanding the ill effects of the undefined behavior we introduce: in > spite of the zero initialization introduced in later rtl stages for the > uninitialized pseudo, by then we've already expanded a PHI node that > referenced the unitialized variable in the path coming from a path in > which it would necessarily be zero, to a copy from the coalesced pseudo, > that gets modified between the zero-initialization and the copy, so the > copied zero is no longer zero. Oops. > > We might want to be stricter in coalesce conflict detection to avoid > this sort of problem, and perhaps to avoid undefined values in uncprop, > but this would all be attempting to limit the effects of undefined > behavior, which is probably a waste of effort. As long as we avoid > introducing undefined behavior ourselves, we shouldn't have to do any of > that. So, this patch fixes loop unswitching so as to not introduce > undefined behavior. > > Regstrapped on x86_64-linux-gnu and i686-linux-gnu. Ok to install? Ok. Thanks, Richard. > > [PR67828] don't unswitch on default defs of non-parms > > From: Alexandre Oliva > > for gcc/ChangeLog > > PR rtl-optimizatoin/67828 > * tree-ssa-loop-unswitch.c: Include tree-ssa.h. > (tree_may_unswitch_on): Don't unswitch on expressions > involving undefined values. > > for gcc/testsuite/ChangeLog > > PR rtl-optimization/67828 > * gcc.dg/torture/pr67828.c: New. > --- > gcc/testsuite/gcc.dg/torture/pr67828.c | 43 ++++++++++++++++++++++++++++++++ > gcc/tree-ssa-loop-unswitch.c | 5 ++++ > 2 files changed, 48 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/torture/pr67828.c > > diff --git a/gcc/testsuite/gcc.dg/torture/pr67828.c b/gcc/testsuite/gcc.dg/torture/pr67828.c > new file mode 100644 > index 0000000..c7b6965 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/torture/pr67828.c > @@ -0,0 +1,43 @@ > +/* Check that we don't misoptimize the final value of d. We used to > + apply loop unswitching on if(j), introducing undefined behavior > + that the original code wouldn't exercise, and this undefined > + behavior would get later passes to misoptimize the loop. */ > + > +/* { dg-do run } */ > + > +#include > +#include > + > +int x; > + > +int __attribute__ ((noinline, noclone)) > +xprintf (int d) { > + if (d) > + { > + if (x) > + printf ("%d", d); > + abort (); > + } > +} > + > +int a, b; > +short c; > + > +int > +main () > +{ > + int j, d = 1; > + for (; c >= 0; c++) > + { > + a = d; > + d = 0; > + if (b) > + { > + xprintf (0); > + if (j) > + xprintf (0); > + } > + } > + xprintf (d); > + exit (0); > +} > diff --git a/gcc/tree-ssa-loop-unswitch.c b/gcc/tree-ssa-loop-unswitch.c > index 4328d6a..d6faa37 100644 > --- a/gcc/tree-ssa-loop-unswitch.c > +++ b/gcc/tree-ssa-loop-unswitch.c > @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3. If not see > #include "internal-fn.h" > #include "gimplify.h" > #include "tree-cfg.h" > +#include "tree-ssa.h" > #include "tree-ssa-loop-niter.h" > #include "tree-ssa-loop.h" > #include "tree-into-ssa.h" > @@ -139,6 +140,10 @@ tree_may_unswitch_on (basic_block bb, struct loop *loop) > /* Condition must be invariant. */ > FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE) > { > + /* Unswitching on undefined values would introduce undefined > + behavior that the original program might never exercise. */ > + if (ssa_undefined_value_p (use, true)) > + return NULL_TREE; > def = SSA_NAME_DEF_STMT (use); > def_bb = gimple_bb (def); > if (def_bb > > > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer