From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113478 invoked by alias); 9 Dec 2015 10:41:48 -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 113459 invoked by uid 89); 9 Dec 2015 10:41:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qg0-f52.google.com Received: from mail-qg0-f52.google.com (HELO mail-qg0-f52.google.com) (209.85.192.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 09 Dec 2015 10:41:46 +0000 Received: by qgcc31 with SMTP id c31so71343867qgc.3 for ; Wed, 09 Dec 2015 02:41:44 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.13.242.133 with SMTP id b127mr5135666ywf.280.1449657704122; Wed, 09 Dec 2015 02:41:44 -0800 (PST) Received: by 10.37.93.11 with HTTP; Wed, 9 Dec 2015 02:41:44 -0800 (PST) In-Reply-To: <20151208162133.GQ3175@redhat.com> References: <20151208162133.GQ3175@redhat.com> Date: Wed, 09 Dec 2015 10:41:00 -0000 Message-ID: Subject: Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949) From: Richard Biener To: Marek Polacek Cc: GCC Patches , Jeff Law , Kugan Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg00976.txt.bz2 On Tue, Dec 8, 2015 at 5:21 PM, Marek Polacek wrote: > The following is a conservative fix for this PR. This is an ICE transpiring > in the new "Factor conversion in COND_EXPR" optimization added in r225722. > > Before this optimization kicks in, we have > : > ... > p1_32 = (short unsigned int) _20; > > : > ... > iftmp.0_18 = (short unsigned int) _20; > > : > ... > # iftmp.0_19 = PHI > > after factor_out_conditional_conversion does its work, we end up with those two > def stmts removed and instead of the PHI we'll have > > # _35 = PHI <_20(3), _20(2)> > iftmp.0_19 = (short unsigned int) _35; > > That itself looks like a fine optimization, but after factor_out_conditional_conversion > there's > 320 phis = phi_nodes (bb2); > 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2); > 322 gcc_assert (phi); > and phis look like > b.2_38 = PHI > _35 = PHI <_20(3), _20(2)> > so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers. > > With this patch we won't ICE (and PRE should clean this up anyway), but I don't know, > maybe I should try harder to optimize even this problematical case (not sure how hard > it would be...)? > > pr66949-2.c only ICEd on powerpc64le and I have verified that this patch fixes it too. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? Hmm. Yes, there is an opportunity for optimization. But I don't see why we have the asserts at all - they seem to be originated from development. IMHO factor_out_conditonal_conversion should simply return the new PHI it generates instead of relying on signle_non_signelton_phi_for_edges to return it. Richard. > 2015-12-08 Marek Polacek > > PR tree-optimization/66949 > * tree-ssa-phiopt.c (factor_out_conditional_conversion): Return false if > NEW_ARG0 and NEW_ARG1 are equal. > > * gcc.dg/torture/pr66949-1.c: New test. > * gcc.dg/torture/pr66949-2.c: New test. > > diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c > index e69de29..1b765bc 100644 > --- gcc/testsuite/gcc.dg/torture/pr66949-1.c > +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c > @@ -0,0 +1,28 @@ > +/* PR tree-optimization/66949 */ > +/* { dg-do compile } */ > + > +int a, *b = &a, c; > + > +unsigned short > +fn1 (unsigned short p1, unsigned int p2) > +{ > + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2; > +} > + > +void > +fn2 () > +{ > + int *d = &a; > + for (a = 0; a < -1; a = 1) > + ; > + if (a < 0) > + c = 0; > + *b = fn1 (*d || c, *d); > +} > + > +int > +main () > +{ > + fn2 (); > + return 0; > +} > diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c > index e69de29..e6250a3 100644 > --- gcc/testsuite/gcc.dg/torture/pr66949-2.c > +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c > @@ -0,0 +1,23 @@ > +/* PR tree-optimization/66949 */ > +/* { dg-do compile } */ > + > +char a; > +int b, c, d; > +extern int fn2 (void); > + > +short > +fn1 (short p1, short p2) > +{ > + return p2 == 0 ? p1 : p1 / p2; > +} > + > +int > +main (void) > +{ > + char e = 1; > + int f = 7; > + c = a >> f; > + b = fn1 (c, 0 < d <= e && fn2 ()); > + > + return 0; > +} > diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c > index 344cd2f..caac5d5 100644 > --- gcc/tree-ssa-phiopt.c > +++ gcc/tree-ssa-phiopt.c > @@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi, > return false; > } > > + /* If we were to continue, we'd create a PHI with same arguments for edges > + E0 and E1. That could get us in trouble later, so punt. */ > + if (operand_equal_for_phi_arg_p (new_arg0, new_arg1)) > + return false; > + > /* If arg0/arg1 have > 1 use, then this transformation actually increases > the number of expressions evaluated at runtime. */ > if (!has_single_use (arg0) > > Marek