From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x530.google.com (mail-pg1-x530.google.com [IPv6:2607:f8b0:4864:20::530]) by sourceware.org (Postfix) with ESMTPS id A6E293858D28 for ; Wed, 3 May 2023 06:28:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A6E293858D28 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-pg1-x530.google.com with SMTP id 41be03b00d2f7-51b603bb360so4116568a12.2 for ; Tue, 02 May 2023 23:28:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683095291; x=1685687291; 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=0CvNHglbOu0evUCkU509Po7XTZo9N8WTZxM+TQHQpaw=; b=In5TV0Gz50tABLDmGWSk00w/Ph4QtGm0HmTz+jkLeH5OdQTSByA5jkjjXvxEHNNDrm 92TeH5lWJJMZPytL4mlWQduEg1flN9KRfYhLhwJaNTZ4lcTJlGgrOjDYJqn+BsYY1fEu znENEY+c8Y8qkfvfMXKjjwwSWsSh4y7Q5m4U/reIVyYClGLs6SvXZ1kadQeBL7eQF/42 5Ucd3WkCqEu34o1fvVE/yr+As7SZf2LkKnZg1NZyUJH3orN2f60I5F8HNG/QthQjHdHK 3hZqH6qHcvVMdN2Bw82CnDcxN0rVVpqvPOGgX5TcZf+9bXJ5GYFYHJBPVJlSQzk1mj58 fKhQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683095291; x=1685687291; 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=0CvNHglbOu0evUCkU509Po7XTZo9N8WTZxM+TQHQpaw=; b=d1T2skAJ4V+rjtHNUlbv2CHxdaffB6zBRtdv29KOfnnfp/p0RVyhxyCKZfQ+Fo2Lpd A6RpAjyj/Uo2Fx1/NKl1L7VWUnInCwp8c3aWQlrmNK457U8GuPqBY3dBp6MxvN6aX9yR kfzRjFWs6jPWCy6K4aEokki3/JURovoyWq6EfPxNDz+BUwBmBjup5AwPg64DGOSnGprH kDsUU5tfIBC2JplOIM4B86AY41lO4dlcoZQIYDVZ6/74HeSuN5VOxsC9uMDRqpVda8WE ZCkmDgZp1ARo3DWGVGy0z+Xy0gJ79sUN5leuGsyALpEfiisAhBgZd7MYDZsY6DdVS10k 5RvA== X-Gm-Message-State: AC+VfDyQYtjUTyLtzlAm0rtGKj3M5nkRYkcTAXxvhxw1rkLnlNAOCQNw Q3pgSG0kuqASteZZjXk+vYOMPIJNfKoZ/0bhyw8= X-Google-Smtp-Source: ACHHUZ5TKnSKlO7+G6Jg8/LLzxk26Bt30F9g4ANqIIs3CA9l+Q04XTg6G7Nt1RYldgDtfVBlzKxjyk4KLVMZen+DkzI= X-Received: by 2002:a05:6a20:9f49:b0:ef:70b:42fc with SMTP id ml9-20020a056a209f4900b000ef070b42fcmr20490915pzb.38.1683095291436; Tue, 02 May 2023 23:28:11 -0700 (PDT) MIME-Version: 1.0 References: <20230430211356.762030-1-apinski@marvell.com> In-Reply-To: From: Andrew Pinski Date: Tue, 2 May 2023 23:27:58 -0700 Message-ID: Subject: Re: [PATCH] PHIOPT: Improve replace_phi_edge_with_variable for diamond shapped bb To: Richard Biener Cc: Andrew Pinski , gcc-patches@gcc.gnu.org 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,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 Tue, May 2, 2023 at 11:14=E2=80=AFPM Richard Biener wrote: > > On Wed, May 3, 2023 at 12:04=E2=80=AFAM Andrew Pinski = wrote: > > > > On Tue, May 2, 2023 at 5:26=E2=80=AFAM Richard Biener via Gcc-patches > > wrote: > > > > > > On Sun, Apr 30, 2023 at 11:14=E2=80=AFPM Andrew Pinski via Gcc-patche= s > > > wrote: > > > > > > > > While looking at differences between what minmax_replacement > > > > and match_simplify_replacement does. I noticed that they sometimes > > > > chose different edges to remove. I decided we should be able to do > > > > better and be able to remove both empty basic blocks in the > > > > case of match_simplify_replacement as that moves the statements. > > > > > > > > This also updates the testcases as now match_simplify_replacement > > > > will remove the unused MIN/MAX_EXPR and they were checking for > > > > those. > > > > > > > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions= . > > > > > > > > gcc/ChangeLog: > > > > > > > > * tree-ssa-phiopt.cc (copy_phi_args): New function. > > > > (replace_phi_edge_with_variable): Handle diamond form bb > > > > with forwarder only empty blocks better. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * gcc.dg/tree-ssa/minmax-15.c: Update test. > > > > * gcc.dg/tree-ssa/minmax-16.c: Update test. > > > > * gcc.dg/tree-ssa/minmax-3.c: Update test. > > > > * gcc.dg/tree-ssa/minmax-4.c: Update test. > > > > * gcc.dg/tree-ssa/minmax-5.c: Update test. > > > > * gcc.dg/tree-ssa/minmax-8.c: Update test. > > > > --- > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-15.c | 3 +- > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-16.c | 9 ++-- > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-3.c | 2 +- > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-4.c | 2 +- > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-5.c | 2 +- > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-8.c | 2 +- > > > > gcc/tree-ssa-phiopt.cc | 51 +++++++++++++++++++= +++- > > > > 7 files changed, 59 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-15.c b/gcc/testsu= ite/gcc.dg/tree-ssa/minmax-15.c > > > > index 8a39871c938..6731f91e6c3 100644 > > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-15.c > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-15.c > > > > @@ -30,5 +30,6 @@ main (void) > > > > return 0; > > > > } > > > > > > > > -/* { dg-final { scan-tree-dump-times "MIN_EXPR" 3 "phiopt1" } } */ > > > > +/* There should only be two MIN_EXPR left, the 3rd one was removed= . */ > > > > +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "phiopt1" } } */ > > > > /* { dg-final { scan-tree-dump-times "MAX_EXPR" 0 "phiopt1" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-16.c b/gcc/testsu= ite/gcc.dg/tree-ssa/minmax-16.c > > > > index 623b12b3f74..094364e6424 100644 > > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-16.c > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-16.c > > > > @@ -25,11 +25,8 @@ main (void) > > > > return 0; > > > > } > > > > > > > > -/* After phiopt1, there really should be only 3 MIN_EXPR in the IR= (including debug statements). > > > > - But the way phiopt does not cleanup the CFG all the time, the P= HI might still reference the > > > > - alternative bb's moved statement. > > > > - Note in the end, we do dce the statement and other debug statem= ents to end up with only 2 MIN_EXPR. > > > > - So check that too. */ > > > > -/* { dg-final { scan-tree-dump-times "MIN_EXPR" 4 "phiopt1" } } */ > > > > +/* After phiopt1, will be only 2 MIN_EXPR in the IR (including deb= ug statements). */ > > > > +/* xk will only have the final result so the extra debug info does= not change anything. */ > > > > +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "phiopt1" } } */ > > > > /* { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "optimized" } } = */ > > > > /* { dg-final { scan-tree-dump-times "MAX_EXPR" 0 "phiopt1" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-3.c b/gcc/testsui= te/gcc.dg/tree-ssa/minmax-3.c > > > > index 2af10776346..521afe3e4d9 100644 > > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-3.c > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-3.c > > > > @@ -25,5 +25,5 @@ main (void) > > > > return 0; > > > > } > > > > > > > > -/* { dg-final { scan-tree-dump-times "MIN_EXPR" 3 "phiopt1" } } */ > > > > +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "phiopt1" } } */ > > > > /* { dg-final { scan-tree-dump-times "MAX_EXPR" 0 "phiopt1" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-4.c b/gcc/testsui= te/gcc.dg/tree-ssa/minmax-4.c > > > > index 973f39bfed3..49e27185b5e 100644 > > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-4.c > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-4.c > > > > @@ -26,4 +26,4 @@ main (void) > > > > } > > > > > > > > /* { dg-final { scan-tree-dump-times "MIN_EXPR" 0 "phiopt1" } } */ > > > > -/* { dg-final { scan-tree-dump-times "MAX_EXPR" 3 "phiopt1" } } */ > > > > +/* { dg-final { scan-tree-dump-times "MAX_EXPR" 2 "phiopt1" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-5.c b/gcc/testsui= te/gcc.dg/tree-ssa/minmax-5.c > > > > index 34e4e720511..194c881cc98 100644 > > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-5.c > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-5.c > > > > @@ -25,5 +25,5 @@ main (void) > > > > return 0; > > > > } > > > > > > > > -/* { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "phiopt1" } } */ > > > > +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "phiopt1" } } */ > > > > /* { dg-final { scan-tree-dump-times "MAX_EXPR" 1 "phiopt1" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-8.c b/gcc/testsui= te/gcc.dg/tree-ssa/minmax-8.c > > > > index 0160e573fef..d5cb53145ea 100644 > > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/minmax-8.c > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-8.c > > > > @@ -26,4 +26,4 @@ main (void) > > > > } > > > > > > > > /* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "phiopt1" } } */ > > > > -/* { dg-final { scan-tree-dump-times "MAX_EXPR" 2 "phiopt1" } } */ > > > > +/* { dg-final { scan-tree-dump-times "MAX_EXPR" 1 "phiopt1" } } */ > > > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > > > > index 65b3deea34a..311423efeb5 100644 > > > > --- a/gcc/tree-ssa-phiopt.cc > > > > +++ b/gcc/tree-ssa-phiopt.cc > > > > @@ -82,6 +82,25 @@ single_non_singleton_phi_for_edges (gimple_seq s= eq, edge e0, edge e1) > > > > return phi; > > > > } > > > > > > > > +/* For each PHI in BB, copy the argument associated with SRC_E to = TGT_E. */ > > > > + > > > > +static void > > > > +copy_phi_args (basic_block bb, edge src_e, edge tgt_e) > > > > +{ > > > > + gphi_iterator gsi; > > > > + int src_indx =3D src_e->dest_idx; > > > > + > > > > + for (gsi =3D gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&g= si)) > > > > + { > > > > + gphi *phi =3D gsi.phi (); > > > > + tree def =3D gimple_phi_arg_def (phi, src_indx); > > > > + location_t locus =3D gimple_phi_arg_location (phi, src_indx)= ; > > > > + > > > > + add_phi_arg (phi, def, tgt_e, locus); > > > > + } > > > > +} > > > > > > Doesn't flush_pending_stmts (tgt_e) do this? > > > > No, In fact the above code is very similar to the code from > > remove_forwarder_block in tree-cfgcleanup.cc (I copied it and changed > > it from copy_phi_args in tree-ssa-threadupdate.cc though as I don't > > need a mapping). > > Let me factor out the code from remove_forwarder_block and put it in > > some common spot and then use that; it will be the same logic even. > > Hmm, but it's odd - if you redirect an edge on GIMPLE then there should > be helpers available to do all this. I think you're doing something wron= g > (without actually looking too close) Maybe some (crude) diagrams are needed to explain why we need to copy the entries for the phi nodes from one edge to another. So the original BB structure is: BB /e1 \e2 BB1 BB2 \e3 /e4 BB3 BB3 has a few phi nodes (except for one of the phi nodes, the entries for BB1, BB2 are all the same). When you redirect e1 (or e2) to BB3, we create new entries in the phi nodes for that edge now as it was not there before. So the shape is: BB |e1 (or e2) BB3 but since it is a new entry in the PHI node, it will be a nullptr. So we need to copy them from the e3 or e4 entries. Does that make sense on why the new function is needed here? This is not a normal operation done by any other pass either. An example of where this API is used in is remove_forwarder_block is doing something similar but instead of 4 BB, it only has 3: BB | e1 BBF (empty forward BB) | e2 BB2 So it redirects e1 to BB2 (bypassing BBF) and the phi nodes need to be copied from e2. Thanks, Andrew > > Richard. > > > Thanks, > > Andrew > > > > > > > > > + > > > > + > > > > /* Replace PHI node element whose edge is E in block BB with varia= ble NEW. > > > > Remove the edge from COND_BLOCK which does not lead to BB (COND= _BLOCK > > > > is known to have two edges, one of which must reach BB). */ > > > > @@ -94,6 +113,7 @@ replace_phi_edge_with_variable (basic_block cond= _block, > > > > basic_block bb =3D gimple_bb (phi); > > > > gimple_stmt_iterator gsi; > > > > tree phi_result =3D PHI_RESULT (phi); > > > > + bool deleteboth =3D false; > > > > > > > > /* Duplicate range info if they are the only things setting the = target PHI. > > > > This is needed as later on, the new_tree will be replacing > > > > @@ -137,7 +157,14 @@ replace_phi_edge_with_variable (basic_block co= nd_block, > > > > keep_edge =3D EDGE_SUCC (cond_block, 1); > > > > } > > > > else if ((keep_edge =3D find_edge (cond_block, e->src))) > > > > - ; > > > > + { > > > > + basic_block bb1 =3D EDGE_SUCC (cond_block, 0)->dest; > > > > + basic_block bb2 =3D EDGE_SUCC (cond_block, 1)->dest; > > > > + if (single_pred_p (bb1) && single_pred_p (bb2) > > > > + && single_succ_p (bb1) && single_succ_p (bb2) > > > > + && empty_block_p (bb1) && empty_block_p (bb2)) > > > > + deleteboth =3D true; > > > > + } > > > > else > > > > gcc_unreachable (); > > > > > > > > @@ -148,6 +175,28 @@ replace_phi_edge_with_variable (basic_block co= nd_block, > > > > e->probability =3D profile_probability::always (); > > > > delete_basic_block (edge_to_remove->dest); > > > > > > > > + /* Eliminate the COND_EXPR at the end of COND_BLOCK. */ > > > > + gsi =3D gsi_last_bb (cond_block); > > > > + gsi_remove (&gsi, true); > > > > + } > > > > + else if (deleteboth) > > > > + { > > > > + basic_block bb1 =3D EDGE_SUCC (cond_block, 0)->dest; > > > > + basic_block bb2 =3D EDGE_SUCC (cond_block, 1)->dest; > > > > + > > > > + edge newedge =3D redirect_edge_and_branch (keep_edge, bb); > > > > + /* no new edge should be the same. */ > > > > + gcc_assert (newedge =3D=3D keep_edge); > > > > + keep_edge->flags |=3D EDGE_FALLTHRU; > > > > + keep_edge->flags &=3D ~(EDGE_TRUE_VALUE | EDGE_FALSE_VALUE); > > > > + keep_edge->probability =3D profile_probability::always (); > > > > + /* Copy the edge's phi entry from the old one */ > > > > + copy_phi_args(bb, e, keep_edge); > > > > + > > > > + /* Delete the old 2 empty basic blocks */ > > > > + delete_basic_block (bb1); > > > > + delete_basic_block (bb2); > > > > + > > > > /* Eliminate the COND_EXPR at the end of COND_BLOCK. */ > > > > gsi =3D gsi_last_bb (cond_block); > > > > gsi_remove (&gsi, true); > > > > -- > > > > 2.31.1 > > > >