Message ID | ZCKkbo4zAg36w3wI@tucnak |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5AE77385840A for <patchwork@sourceware.org>; Tue, 28 Mar 2023 08:25:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5AE77385840A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1679991955; bh=gRzMkpUIF233bAPjwXHmkBGt1ndNU7rQshgpxVvD420=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=MYSEqaByMibXmAWwmR3I9xhj+ITrIgfwrv7dBeeppMQBG08awCOBUdHvd+JPy+m9Z iGBZX0fQVrmVlWBCRmPWAvSEo2mCvZcuIRtYIvfaKElYkSETHDbT/qk3kkJSEV1pWa jfEMrMQeQ4jOa3jnAE8flT1ZHRs0Hvf8u3vZpCog= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id DB8563858D39 for <gcc-patches@gcc.gnu.org>; Tue, 28 Mar 2023 08:25:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DB8563858D39 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-186-aJwOOzpVNLKR1nvu5B_xWA-1; Tue, 28 Mar 2023 04:25:22 -0400 X-MC-Unique: aJwOOzpVNLKR1nvu5B_xWA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D57CF385F36F; Tue, 28 Mar 2023 08:25:21 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 91E944020C82; Tue, 28 Mar 2023 08:25:21 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 32S8PJfk1442108 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 28 Mar 2023 10:25:19 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 32S8PIRb1442107; Tue, 28 Mar 2023 10:25:18 +0200 Date: Tue, 28 Mar 2023 10:25:18 +0200 To: Richard Biener <rguenther@suse.de> Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] Message-ID: <ZCKkbo4zAg36w3wI@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301]
|
|
Commit Message
Jakub Jelinek
March 28, 2023, 8:25 a.m. UTC
Hi! The following testcase ICEs since the sincos and vect pass order has been swapped. It is not valid to replace vector sqrt (sqrt (x)) with pow (x, 0.25) because build_real on vector type is invalid (could be handled by using build_uniform_cst and adjusting type passed to build_real) but more importantly because nothing checks if we can actually do vector pow. While we have pow_optab, apparently no target defines it, so it doesn't seem to be worth bothering with for now and the patch just punts on non-scalar sqrts. I think the other simplifications next to it are fine, as they mostly use CBRT which doesn't even have internal function (so is a builtin only and therefore always scalar), or have already pow in the IL (which doesn't have optab and shouldn't be thus vector either). It is true that with <bits/math-vector.h> we do vectorize some calls to pow or cbrt (but don't handle others strangely), but those aren't using internal functions but simd clones and so match.pd doesn't know anything about those (at least for now). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2023-03-28 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/109301 * match.pd (sqrt (sqrt (x)) -> pow (x, 0.25)): Only simplify for scalar floating point types. * gcc.dg/pr109301.c: New test. Jakub
Comments
On Tue, 28 Mar 2023, Jakub Jelinek wrote: > Hi! > > The following testcase ICEs since the sincos and vect pass order has > been swapped. It is not valid to replace vector sqrt (sqrt (x)) with > pow (x, 0.25) because build_real on vector type is invalid (could be > handled by using build_uniform_cst and adjusting type passed to > build_real) but more importantly because nothing checks if we can > actually do vector pow. > While we have pow_optab, apparently no target defines it, so it doesn't > seem to be worth bothering with for now and the patch just punts on > non-scalar sqrts. > I think the other simplifications next to it are fine, as they mostly > use CBRT which doesn't even have internal function (so is a builtin > only and therefore always scalar), or have already pow in the IL (which > doesn't have optab and shouldn't be thus vector either). > It is true that with <bits/math-vector.h> we do vectorize some calls to > pow or cbrt (but don't handle others strangely), but those aren't using > internal functions but simd clones and so match.pd doesn't know anything > about those (at least for now). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Hmm, but canonicalize_math_p () should be false after vectorization? When we moved the pass we should have made sure to put the PROP_gimple_opt_math property set to pass_expand_powcabs instead. Now, the sqrt (sqrt ()) canonicalization to pow (.., 1./4) is probably invalid anyway, not sure if we can add a user-written vector sqrt testcase that would trigger during the canonicalization phase. There are other uses of build_real that have the same issue - what's your conclusion this is never a problem there? Thanks, Richard. > 2023-03-28 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/109301 > * match.pd (sqrt (sqrt (x)) -> pow (x, 0.25)): Only simplify for > scalar floating point types. > > * gcc.dg/pr109301.c: New test. > > --- gcc/match.pd.jj 2023-03-27 10:25:40.171676370 +0200 > +++ gcc/match.pd 2023-03-27 21:03:38.816171522 +0200 > @@ -6820,7 +6820,8 @@ (define_operator_list SYNC_FETCH_AND_AND > /* sqrt(sqrt(x)) -> pow(x,1/4). */ > (simplify > (sqrts (sqrts @0)) > - (pows @0 { build_real (type, dconst_quarter ()); })) > + (if (SCALAR_FLOAT_TYPE_P (type)) > + (pows @0 { build_real (type, dconst_quarter ()); }))) > /* sqrt(cbrt(x)) -> pow(x,1/6). */ > (simplify > (sqrts (cbrts @0)) > --- gcc/testsuite/gcc.dg/pr109301.c.jj 2023-03-27 21:06:22.635806234 +0200 > +++ gcc/testsuite/gcc.dg/pr109301.c 2023-03-27 21:06:05.413054906 +0200 > @@ -0,0 +1,13 @@ > +/* PR tree-optimization/109301 */ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -ffast-math" } */ > + > +double x[256]; > + > +void > +foo (void) > +{ > + for (int i = 0; i < 256; ++i) > + for (int j = 0; j < 8; ++j) > + x[i] = __builtin_pow (x[i], 0.5); > +} > > Jakub > >
On Tue, Mar 28, 2023 at 08:57:12AM +0000, Richard Biener wrote: > Hmm, but canonicalize_math_p () should be false after vectorization? > > When we moved the pass we should have made sure to put the > PROP_gimple_opt_math property set to pass_expand_powcabs instead. Which pass is the one that actually canonicalizes the math such that we want to keep its choices for later? I must say I don't know the details why the sincos path has been even moved. > Now, the sqrt (sqrt ()) canonicalization to pow (.., 1./4) is > probably invalid anyway, not sure if we can add a user-written > vector sqrt testcase that would trigger during the canonicalization How do we write user written vector sqrt? > phase. There are other uses of build_real that have the same > issue - what's your conclusion this is never a problem there? Looking through build_real* calls in match.pd, others are either on simplifications of some expressions with REAL_CST operand (so can't be vector then), or using LOG*/EXP*/CBRT*/SIN*/ATAN*/COS*/POW*/CABS*/HYPOT*/POWI*/SIGNBIT* calls in the expression being simplified, or this case. I think no target provides vector optabs for those or they don't have internal fns at all. Maybe (simplify /* signbit(x) -> x<0 if x doesn't have signed zeros. */ (SIGNBIT @0) (if (!HONOR_SIGNED_ZEROS (@0)) (convert (lt @0 { build_real (TREE_TYPE (@0), dconst0); })))) ? Jakub
On Tue, 28 Mar 2023, Jakub Jelinek wrote: > On Tue, Mar 28, 2023 at 08:57:12AM +0000, Richard Biener wrote: > > Hmm, but canonicalize_math_p () should be false after vectorization? > > > > When we moved the pass we should have made sure to put the > > PROP_gimple_opt_math property set to pass_expand_powcabs instead. > > Which pass is the one that actually canonicalizes the math such > that we want to keep its choices for later? > I must say I don't know the details why the sincos path has been > even moved. The pass was split into sincos + pass_expand_powcabs - canonicalization happens through folding and pass_expand_powcabs expands pow (x, 2) to x * x. Folding would make x * x to pow (x, 2) so it's important to set the property after pass_expand_powcabs. I think we moved sincos because vectorizing cexpi never materialized(?) but maybe I misremember. > > Now, the sqrt (sqrt ()) canonicalization to pow (.., 1./4) is > > probably invalid anyway, not sure if we can add a user-written > > vector sqrt testcase that would trigger during the canonicalization > > How do we write user written vector sqrt? I'm not sure ;) > > phase. There are other uses of build_real that have the same > > issue - what's your conclusion this is never a problem there? > > Looking through build_real* calls in match.pd, others are > either on simplifications of some expressions with REAL_CST operand > (so can't be vector then), or using > LOG*/EXP*/CBRT*/SIN*/ATAN*/COS*/POW*/CABS*/HYPOT*/POWI*/SIGNBIT* > calls in the expression being simplified, or this case. > I think no target provides vector optabs for those or they don't > have internal fns at all. > Maybe > (simplify > /* signbit(x) -> x<0 if x doesn't have signed zeros. */ > (SIGNBIT @0) > (if (!HONOR_SIGNED_ZEROS (@0)) > (convert (lt @0 { build_real (TREE_TYPE (@0), dconst0); })))) > ? Maybe. But as said, the fix is probably to move the pass property. Richard.
--- gcc/match.pd.jj 2023-03-27 10:25:40.171676370 +0200 +++ gcc/match.pd 2023-03-27 21:03:38.816171522 +0200 @@ -6820,7 +6820,8 @@ (define_operator_list SYNC_FETCH_AND_AND /* sqrt(sqrt(x)) -> pow(x,1/4). */ (simplify (sqrts (sqrts @0)) - (pows @0 { build_real (type, dconst_quarter ()); })) + (if (SCALAR_FLOAT_TYPE_P (type)) + (pows @0 { build_real (type, dconst_quarter ()); }))) /* sqrt(cbrt(x)) -> pow(x,1/6). */ (simplify (sqrts (cbrts @0)) --- gcc/testsuite/gcc.dg/pr109301.c.jj 2023-03-27 21:06:22.635806234 +0200 +++ gcc/testsuite/gcc.dg/pr109301.c 2023-03-27 21:06:05.413054906 +0200 @@ -0,0 +1,13 @@ +/* PR tree-optimization/109301 */ +/* { dg-do compile } */ +/* { dg-options "-O3 -ffast-math" } */ + +double x[256]; + +void +foo (void) +{ + for (int i = 0; i < 256; ++i) + for (int j = 0; j < 8; ++j) + x[i] = __builtin_pow (x[i], 0.5); +}