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 40C4C3858428 for ; Mon, 13 Nov 2023 11:53:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 40C4C3858428 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 40C4C3858428 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::12d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699876393; cv=none; b=eMqYZaYtbRvYtRZm7iz4j6/B38QN/I6QVRN5V07bVCKaZiIkTSKQwtd8C/+/WKvddVvaBIyJQXtEO30hKG+4z429xljOd8d0iFB7wOOXhnX2dfe+2meHQHzSzbAAXoSmpVXWr6MzZrGf5qHHE9pP10KlueATTCB+ffp+i9rIPjk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699876393; c=relaxed/simple; bh=cS8ae06ZjUZfWm3aajMpv/hzkyoAuAQxH1cm590vYgs=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=APgx1bcmpnCr05+JQI/GR8QQbgu47X0QlT7f6CdrOoMCIAetFak8Ti8a17uzQFlLd8+fSlIxoiPz+qAsF7ZqmSGHTZdGbbVgJYGhBSpWaY1059Z880IsMfma6SU38bas5Ht68U91mDVCEDazFlOycIjZZQg/YQnYEWNHfpnLx0c= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x12d.google.com with SMTP id 2adb3069b0e04-5098eb6690cso5592909e87.3 for ; Mon, 13 Nov 2023 03:53:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699876390; x=1700481190; darn=gcc.gnu.org; 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=T04PztC3F5PSeGNag2AfhtNhBA+lxCS3QI0Vt9TY3Rs=; b=le+ynUBodazaXb98sWGxc70MuZ+1oPN5NxDqdin0rhRIaH02eTtAkKFET7EZc3Q0SD u8z4S4Gl/zaWlTk51o7yRMZFhLBDqfCaZNTJ9JulI8RLkCuspW+8K0k9lKPO77symKu8 NMPrz1baJ0E15+vW8Yo0LXKLzfisfTGvUuiwqkI504I7d86ErjSUoyitsQRpP8nm8AnZ apSktuadqyElReafJ0gpxbrEpQR+O5aSj3RvPxJwykXUwS0T57snT9+3LVRVa2EhaBYe W/6smVtJoP802JDDh+mCmtobYLqzW3hAdMUxTTHAA6j/bl/RNgE5NS0cssSR6iTUCq7s KQpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699876390; x=1700481190; 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=T04PztC3F5PSeGNag2AfhtNhBA+lxCS3QI0Vt9TY3Rs=; b=pN5x56r97ZZeBWGCiJq++LZ6sakd6CNghqw8RMjcDC9rtmM+BOVZnMgqk7fQkH6045 y86NUyOTyBrPhM2uEM9GAnyHCESvwRI+QXLdhrO11joBXTPZtyQyxFNJcekXYiru1T4l hhSpyBWa5RTQXKxpHuaNHaDuziBg/4sZbTvd4ArxfK+QlToFnOJVwp7D+aa7K8PuXV5j frgrTOQb635KSjTgMB+Iwx/2a9r/28WyIs0gwH1jV7lwLBIa/hFZglR1Mz/SqyCFVryj s8gBWUiaMV+B8fshVz7iLkTUrrDrtI7QwiZqprtDONr+7Vvg9VX6nQ2mgyBGapwSJ192 zZxw== X-Gm-Message-State: AOJu0Yxw/h68msauTIgcwNoz3SdA2QjdQDmK1dtiKdjAaWhT/fZB4+wz 7YBZ1BZq0Tn6V5yuyHcA3CiYJqHDD09vabSw3Yo= X-Google-Smtp-Source: AGHT+IGssTlsJ8bPCVLwxeDvpOpPELJtZ+hFb9gMAqzKKigLpEqd2Tc1bIzf86uKlOioEUdfPx2uu9Gb34H98Bg9WdQ= X-Received: by 2002:a19:5f46:0:b0:500:daec:2817 with SMTP id a6-20020a195f46000000b00500daec2817mr4154845lfj.54.1699876389375; Mon, 13 Nov 2023 03:53:09 -0800 (PST) MIME-Version: 1.0 References: <20231030104145.169132-1-hongtao.liu@intel.com> <20231107060539.443303-1-hongtao.liu@intel.com> In-Reply-To: From: Richard Biener Date: Mon, 13 Nov 2023 12:52:56 +0100 Message-ID: Subject: Re: [V2 PATCH] Handle bitop with INTEGER_CST in analyze_and_compute_bitop_with_inv_effect. To: Hongtao Liu Cc: liuhongt , gcc-patches@gcc.gnu.org, hjl.tools@gmail.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,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 Mon, Nov 13, 2023 at 8:50=E2=80=AFAM Hongtao Liu wr= ote: > > On Fri, Nov 10, 2023 at 5:12=E2=80=AFPM Richard Biener > wrote: > > > > On Wed, Nov 8, 2023 at 9:22=E2=80=AFAM Hongtao Liu = wrote: > > > > > > On Wed, Nov 8, 2023 at 3:53=E2=80=AFPM Richard Biener > > > wrote: > > > > > > > > On Wed, Nov 8, 2023 at 2:18=E2=80=AFAM Hongtao Liu wrote: > > > > > > > > > > On Tue, Nov 7, 2023 at 10:34=E2=80=AFPM Richard Biener > > > > > wrote: > > > > > > > > > > > > On Tue, Nov 7, 2023 at 2:03=E2=80=AFPM Hongtao Liu wrote: > > > > > > > > > > > > > > On Tue, Nov 7, 2023 at 4:10=E2=80=AFPM Richard Biener > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, Nov 7, 2023 at 7:08=E2=80=AFAM liuhongt wrote: > > > > > > > > > > > > > > > > > > analyze_and_compute_bitop_with_inv_effect assumes the fir= st operand is > > > > > > > > > loop invariant which is not the case when it's INTEGER_CS= T. > > > > > > > > > > > > > > > > > > Bootstrapped and regtseted on x86_64-pc-linux-gnu{-m32,}. > > > > > > > > > Ok for trunk? > > > > > > > > > > > > > > > > So this addresses a missed optimization, right? It seems t= o me that > > > > > > > > even with two SSA names we are only "lucky" when rhs1 is th= e invariant > > > > > > > > one. So instead of swapping this way I'd do > > > > > > > Yes, it's a miss optimization. > > > > > > > And I think expr_invariant_in_loop_p (loop, match_op[1]) shou= ld be > > > > > > > enough, if match_op[1] is a loop invariant.it must be false f= or the > > > > > > > below conditions(there couldn't be any header_phi from its > > > > > > > definition). > > > > > > > > > > > > Yes, all I said is that when you now care for op1 being INTEGER= _CST > > > > > > it could also be an invariant SSA name and thus only after swap= ping op0/op1 > > > > > > we could have a successful match, no? > > > > > Sorry, the commit message is a little bit misleading. > > > > > At first, I just wanted to handle the INTEGER_CST case (with TREE= _CODE > > > > > (match_op[1]) =3D=3D INTEGER_CST), but then I realized that this = could > > > > > probably be extended to the normal SSA_NAME case as well, so I us= ed > > > > > expr_invariant_in_loop_p, which should theoretically be able to h= andle > > > > > the SSA_NAME case as well. > > > > > > > > > > if (expr_invariant_in_loop_p (loop, match_op[1])) is true, w/o > > > > > swapping it must return NULL_TREE for below conditions. > > > > > if (expr_invariant_in_loop_p (loop, match_op[1])) is false, w/ > > > > > swapping it must return NULL_TREE too. > > > > > So it can cover the both cases you mentioned, no need for a loop = to > > > > > iterate 2 match_ops for all conditions. > > > > > > > > Sorry if it appears we're going in circles ;) > > > > > > > > > 3692 if (TREE_CODE (match_op[1]) !=3D SSA_NAME > > > > > 3693 || !expr_invariant_in_loop_p (loop, match_op[0]) > > > > > 3694 || !(header_phi =3D dyn_cast (SSA_NAME_DEF_STM= T (match_op[1]))) > > > > > > > > but this only checks match_op[1] (an SSA name at this point) for be= ing defined > > > > by the header PHI. What if expr_invariant_in_loop_p (loop, mach_op= [1]) > > > > and header_phi =3D dyn_cast (SSA_NAME_DEF_STMT (match_op[0= ])) > > > > which I think can happen when both ops are SSA name? > > > The whole condition is like > > > > > > 3692 if (TREE_CODE (match_op[1]) !=3D SSA_NAME > > > 3693 || !expr_invariant_in_loop_p (loop, match_op[0]) > > > 3694 || !(header_phi =3D dyn_cast (SSA_NAME_DEF_STMT (m= atch_op[1]))) > > > 3695 || gimple_bb (header_phi) !=3D loop->header ----- This wou= ld > > > be true if match_op[1] is SSA_NAME and expr_invariant_in_loop_p > > > > But it could be expr_invariant_in_loop_p (match_op[1]) and > > header_phi =3D dyn_cast (SSA_NAME_DEF_STMT (match_op[0])) > > > > > > > > > > + if (expr_invariant_in_loop_p (loop, match_op[1])) > > > > > > > > > + std::swap (match_op[0], match_op[1]); > match_op[1] will be swapped to match_op[0], the case is also handled > by my patch [1](the v2 patch) > My point is the upper code already handles 2 SSA names, no need to > iterate with all conditions, expr_invariant_in_loop_p alone is enough. > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635440.html I see - thanks for the repeated explanations. That patch is OK. Thanks, Richard. > > > > all I say is that for two SSA names we could not match the condition > > (aka not fail) > > when we swap op0/op1. Not only when op1 is INTEGER_CST. > > > > > > 3696 || gimple_phi_num_args (header_phi) !=3D 2) > > > > > > If expr_invariant_in_loop_p (loop, mach_op[1]) is true and it's an SS= A_NAME > > > according to code in expr_invariant_in_loop_p, def_bb of gphi is > > > either NULL or not belong to this loop, either case will make will > > > make gimple_bb (header_phi) !=3D loop->header true. > > > > > > 1857 if (TREE_CODE (expr) =3D=3D SSA_NAME) > > > 1858 { > > > 1859 def_bb =3D gimple_bb (SSA_NAME_DEF_STMT (expr)); > > > 1860 if (def_bb > > > 1861 && flow_bb_inside_loop_p (loop, def_bb)) -- def_bb is > > > NULL or it doesn't belong to the loop > > > 1862 return false; > > > 1863 > > > 1864 return true; > > > 1865 } > > > 1866 > > > 1867 if (!EXPR_P (expr)) > > > > > > > > > > > The only canonicalization we have is that constant operands are put= second so > > > > it would have been more natural to write the matching with the othe= r operand > > > > order (but likely you'd have been unlucky for the existing testcase= s then). > > > > > > > > > 3695 || gimple_bb (header_phi) !=3D loop->header > > > > > 3696 || gimple_phi_num_args (header_phi) !=3D 2) > > > > > 3697 return NULL_TREE; > > > > > 3698 > > > > > 3699 if (PHI_ARG_DEF_FROM_EDGE (header_phi, loop_latch_edge (loo= p)) !=3D phidef) > > > > > 3700 return NULL_TREE; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > unsigned i; > > > > > > > > for (i =3D 0; i < 2; ++i) > > > > > > > > if (TREE_CODE (match_op[i]) =3D=3D SSA_NAME > > > > > > > > && ...) > > > > > > > > break; /* found! */ > > > > > > > > > > > > > > > > if (i =3D=3D 2) > > > > > > > > return NULL_TREE; > > > > > > > > if (i =3D=3D 0) > > > > > > > > std::swap (match_op[0], match_op[1]); > > > > > > > > > > > > > > > > to also handle a "swapped" pair of SSA names? > > > > > > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > > > > > > > PR tree-optimization/105735 > > > > > > > > > PR tree-optimization/111972 > > > > > > > > > * tree-scalar-evolution.cc > > > > > > > > > (analyze_and_compute_bitop_with_inv_effect): Hand= le bitop with > > > > > > > > > INTEGER_CST. > > > > > > > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > > > > > > > * gcc.target/i386/pr105735-3.c: New test. > > > > > > > > > --- > > > > > > > > > gcc/testsuite/gcc.target/i386/pr105735-3.c | 87 ++++++++= ++++++++++++++ > > > > > > > > > gcc/tree-scalar-evolution.cc | 3 + > > > > > > > > > 2 files changed, 90 insertions(+) > > > > > > > > > create mode 100644 gcc/testsuite/gcc.target/i386/pr10573= 5-3.c > > > > > > > > > > > > > > > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr105735-3.c b= /gcc/testsuite/gcc.target/i386/pr105735-3.c > > > > > > > > > new file mode 100644 > > > > > > > > > index 00000000000..9e268a1a997 > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/gcc/testsuite/gcc.target/i386/pr105735-3.c > > > > > > > > > @@ -0,0 +1,87 @@ > > > > > > > > > +/* { dg-do compile } */ > > > > > > > > > +/* { dg-options "-O1 -fdump-tree-sccp-details" } */ > > > > > > > > > +/* { dg-final { scan-tree-dump-times {final value replac= ement} 8 "sccp" } } */ > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +foo (unsigned int tmp) > > > > > > > > > +{ > > > > > > > > > + for (int bit =3D 0; bit < 64; bit++) > > > > > > > > > + tmp &=3D 11304; > > > > > > > > > + return tmp; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +foo1 (unsigned int tmp) > > > > > > > > > +{ > > > > > > > > > + for (int bit =3D 63; bit >=3D 0; bit -=3D3) > > > > > > > > > + tmp &=3D 11304; > > > > > > > > > + return tmp; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +foo2 (unsigned int tmp) > > > > > > > > > +{ > > > > > > > > > + for (int bit =3D 0; bit < 64; bit++) > > > > > > > > > + tmp |=3D 11304; > > > > > > > > > + return tmp; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +foo3 (unsigned int tmp) > > > > > > > > > +{ > > > > > > > > > + for (int bit =3D 63; bit >=3D 0; bit -=3D3) > > > > > > > > > + tmp |=3D 11304; > > > > > > > > > + return tmp; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +foo4 (unsigned int tmp) > > > > > > > > > +{ > > > > > > > > > + for (int bit =3D 0; bit < 64; bit++) > > > > > > > > > + tmp ^=3D 11304; > > > > > > > > > + return tmp; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +foo5 (unsigned int tmp) > > > > > > > > > +{ > > > > > > > > > + for (int bit =3D 0; bit < 63; bit++) > > > > > > > > > + tmp ^=3D 11304; > > > > > > > > > + return tmp; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +f (unsigned int tmp, int bit) > > > > > > > > > +{ > > > > > > > > > + unsigned int res =3D tmp; > > > > > > > > > + for (int i =3D 0; i < bit; i++) > > > > > > > > > + res &=3D 11304; > > > > > > > > > + return res; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +f1 (unsigned int tmp, int bit) > > > > > > > > > +{ > > > > > > > > > + unsigned int res =3D tmp; > > > > > > > > > + for (int i =3D 0; i < bit; i++) > > > > > > > > > + res |=3D 11304; > > > > > > > > > + return res; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +unsigned int > > > > > > > > > +__attribute__((noipa)) > > > > > > > > > +f2 (unsigned int tmp, int bit) > > > > > > > > > +{ > > > > > > > > > + unsigned int res =3D tmp; > > > > > > > > > + for (int i =3D 0; i < bit; i++) > > > > > > > > > + res ^=3D 11304; > > > > > > > > > + return res; > > > > > > > > > +} > > > > > > > > > diff --git a/gcc/tree-scalar-evolution.cc b/gcc/tree-scal= ar-evolution.cc > > > > > > > > > index 70b17c5bca1..f61277c32df 100644 > > > > > > > > > --- a/gcc/tree-scalar-evolution.cc > > > > > > > > > +++ b/gcc/tree-scalar-evolution.cc > > > > > > > > > @@ -3689,6 +3689,9 @@ analyze_and_compute_bitop_with_inv_= effect (class loop* loop, tree phidef, > > > > > > > > > match_op[0] =3D gimple_assign_rhs1 (def); > > > > > > > > > match_op[1] =3D gimple_assign_rhs2 (def); > > > > > > > > > > > > > > > > > > + if (expr_invariant_in_loop_p (loop, match_op[1])) > > > > > > > > > + std::swap (match_op[0], match_op[1]); > > > > > > > > > + > > > > > > > > > if (TREE_CODE (match_op[1]) !=3D SSA_NAME > > > > > > > > > || !expr_invariant_in_loop_p (loop, match_op[0]) > > > > > > > > > || !(header_phi =3D dyn_cast (SSA_NAME_DE= F_STMT (match_op[1]))) > > > > > > > > > -- > > > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > BR, > > > > > > > Hongtao > > > > > > > > > > > > > > > > > > > > -- > > > > > BR, > > > > > Hongtao > > > > > > > > > > > > -- > > > BR, > > > Hongtao > > > > -- > BR, > Hongtao