From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com [IPv6:2a00:1450:4864:20::12d]) by sourceware.org (Postfix) with ESMTPS id 1D9543858D32 for ; Mon, 8 May 2023 06:52:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1D9543858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x12d.google.com with SMTP id 2adb3069b0e04-4f139de8cefso26015450e87.0 for ; Sun, 07 May 2023 23:52:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683528720; x=1686120720; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=lQSDWmCLKVefMN4ppmcWybTBN+rtuGEIhjpczfjgDVc=; b=olH3iASzxK5VyghWJmE1oauMZy/u1cfxy5nBlBFybmx3bUH7oAsr0QBGKJuLyCFktG cqUmQGzqoJlzE+UBrv+z8mpqx3zaMAUjCCE1m76zpgomcO8e7hOEzIh8qDByVsXK+Q+O +h+H9SPyTvuPh4VK3vm9cv+aTQXyqMiNYByOrIhxetlm1WKJlvVdUH0d4ya6c7ezXvim eUH9UHi0VAFa0TLuP6NmEYS5SI35wfswsFmV144bkLGgjfKVbB0dKD2z6jlP7wtOQfEY +OlCS18D/N0bSV/le5GZIwFXJ4Mxz2sWqnYVBNWu5G8XrR5eBNmr7mDKMqesQRYeexzD 1gFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683528720; x=1686120720; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=lQSDWmCLKVefMN4ppmcWybTBN+rtuGEIhjpczfjgDVc=; b=ZEx+iMj6M3IrSwH5pZ80enrkwpP7sTygovtQ0Ud+HPkjCUsy/BWErE6s++8mX4OadD 4MSBayjGhTTf3ZPxYLh/mjlPkPdWkWn4fNWjSisG5s4CBjM9cn4F/m47c0bsuKv7ZIpO 1G/GqI2m7tU49wjyFiuEkQj/P61MUpTjGYRl/5J3/pgiMgPiBQo9v9XzcPqd3VYLrvbs Gw5SosxBFD2S+h5/OzzJSxgkl+ODZlPMXNXVa5VFTTqs9IkW0qyZDQXCDpGaJ0Om6pbY srRChX9c4qyH7+McKyxYT35IO/Jg0HZIy84oo/a/HIji4YJtdPyD3heckT2Owu4yK5zE 5MKA== X-Gm-Message-State: AC+VfDzKJkMqUUZ0XWchpYGpHAzNLSSPagDkdzvdrnkiHikRD0Sckp4A IbfJAZwNwrff+FgGbeQET5aZUTx9mh9OQvtACSYibhEX X-Google-Smtp-Source: ACHHUZ71HlQyv9MfqUOkmA9zIFOZ1MbemoxuM/eJB4h4P7PYTJW/NW8p9DYswno+UR4R59UUU3zF7sPzBOoNaV4gLeg= X-Received: by 2002:a2e:b5d4:0:b0:2a9:45fb:6331 with SMTP id g20-20020a2eb5d4000000b002a945fb6331mr2504393ljn.6.1683528719615; Sun, 07 May 2023 23:51:59 -0700 (PDT) MIME-Version: 1.0 References: <20230507044332.1122612-1-apinski@marvell.com> In-Reply-To: <20230507044332.1122612-1-apinski@marvell.com> From: Richard Biener Date: Mon, 8 May 2023 08:51:47 +0200 Message-ID: Subject: Re: [PATCH 1/3] PHIOPT: Add diamond bb form to factor_out_conditional_conversion To: Andrew Pinski Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sun, May 7, 2023 at 7:05=E2=80=AFAM Andrew Pinski via Gcc-patches wrote: > > So the function factor_out_conditional_conversion already supports > diamond shaped bb forms, just need to be called for such a thing. > > harden-cond-comp.c needed to be changed as we would optimize out the > conversion now and that causes the compare hardening not needing to > split the block which it was testing. So change it such that there > would be no chance of optimization. > > Also add two testcases that showed the improvement. PR 103771 is > solved in ifconvert also for the vectorizer but now it is solved > in a general sense. > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. So what does it do for the diamond case? Factor out _common_ operations only or also promote conditionally executed operations to unconditionally executed? Apart from cost considerations (with the looping patch any numbe= r might end up promoted) there's correctness issues for negation (signed over= flow) and for non-call exceptions there's possible external throws. If it only factors out common operations the patch is OK (but the testcases suggest that's not the case). Otherwise we should add a limit as to how ma= ny ops we hoist (maybe one for the diamond case?). Thinking over it the same issue of course exists for the non-diamond case? Richard. > PR tree-optimization/49959 > PR tree-optimization/103771 > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (pass_phiopt::execute): Support > Diamond shapped bb form for factor_out_conditional_conversion. > > gcc/testsuite/ChangeLog: > > * c-c++-common/torture/harden-cond-comp.c: Change testcase > slightly to avoid the new phiopt optimization. > * gcc.dg/tree-ssa/abs-2.c: New test. > * gcc.dg/tree-ssa/pr103771.c: New test. > --- > .../c-c++-common/torture/harden-cond-comp.c | 6 +++--- > gcc/testsuite/gcc.dg/tree-ssa/abs-2.c | 20 +++++++++++++++++++ > gcc/testsuite/gcc.dg/tree-ssa/pr103771.c | 18 +++++++++++++++++ > gcc/tree-ssa-phiopt.cc | 2 +- > 4 files changed, 42 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/abs-2.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr103771.c > > diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/= testsuite/c-c++-common/torture/harden-cond-comp.c > index 5aad890a1d3..dcf364ee993 100644 > --- a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c > +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c > @@ -1,11 +1,11 @@ > /* { dg-do compile } */ > /* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-= tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */ > > -int f(int i, int j) { > +int f(int i, int j, int k, int l) { > if (i =3D=3D 0) > - return j !=3D 0; > + return (j !=3D 0) + l; > else > - return i * j !=3D 0; > + return (i * j !=3D 0) * k; > } > > /* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */ > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c b/gcc/testsuite/gcc.dg= /tree-ssa/abs-2.c > new file mode 100644 > index 00000000000..328b1802541 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/abs-2.c > @@ -0,0 +1,20 @@ > +/* PR tree-optimization/49959 */ > +/* { dg-do compile } */ > +/* { dg-options "-O -fdump-tree-phiopt1-details" } */ > + > +#define ABS(X) (((X)>0)?(X):-(X)) > +unsigned long > +test_abs(int *cur) > +{ > + unsigned long sad =3D 0; > + if (cur[0] > 0) > + sad =3D cur[0]; > + else > + sad =3D -cur[0]; > + return sad; > +} > + > +/* We should figure out that test_abs has an ABS_EXPR in it. */ > +/* { dg-final { scan-tree-dump " =3D ABS_EXPR" "phiopt1"} } */ > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out f= rom" 1 "phiopt1"} } */ > + > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c b/gcc/testsuite/gcc= .dg/tree-ssa/pr103771.c > new file mode 100644 > index 00000000000..97c9db846cb > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr103771.c > @@ -0,0 +1,18 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -fdump-tree-phiopt1-details" } */ > +/* { dg-final { scan-tree-dump-times "changed to factor conversion out f= rom COND_EXPR." 1 "phiopt1" } } */ > + > +typedef unsigned char uint8_t; > + > +static uint8_t x264_clip_uint8 (int x) > +{ > + return x & (~255) ? (-x) >> 31 : x; > +} > + > +void > +mc_weight (uint8_t* __restrict dst, uint8_t* __restrict src, > + int i_width,int i_scale) > +{ > + for(int x =3D 0; x < i_width; x++) > + dst[x] =3D x264_clip_uint8 (src[x] * i_scale); > +} > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index f14b7e8b7e6..41fea78dc8d 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -4087,7 +4087,7 @@ pass_phiopt::execute (function *) > > gphi *newphi; > if (single_pred_p (bb1) > - && !diamond_p > + && EDGE_COUNT (merge->preds) =3D=3D 2 > && (newphi =3D factor_out_conditional_conversion (e1, e2, phi, > arg0, arg1, > cond_stmt))) > -- > 2.31.1 >