From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by sourceware.org (Postfix) with ESMTPS id 51A163858C50 for ; Mon, 24 Apr 2023 12:07:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 51A163858C50 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-x12b.google.com with SMTP id 2adb3069b0e04-4eff1f7f34bso857801e87.0 for ; Mon, 24 Apr 2023 05:07:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682338072; x=1684930072; 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=VX1/kosqWpxwBOx4VlQL+H4XvhWpo3gVcK51kC20+sM=; b=SZtDiVKRFgbHBcA7U4ZR59NgMJRS2wT1TqIoUNl1Jxb2E+kwNj2U9ZxvMYb2fh25Fb s0IOm8C/wwsYifnN97QolVUb1cAGNoYZj0XprWlOtxL//giDjjF/vmu+5mvdC7mpalUa 0OWmzDv9DQAZfcW2fZcu4jQN6VcTM4VxUhtIE1j6VvnKptso6qiZlmxUkems1zECeE2X XakIwwJNdClYoq4/6Ukd+IpjP+hhe/6qnWIJuGRjtGd5rvgpiko6BcY3xkaoTvGTBfz4 nh63B0/zWkbwWsqCHUhM4Wkf2KVafb8lFEbdQRgm1+MJvRZ1yKA0/mQfR+Lr1s7ARgc1 v+gA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682338072; x=1684930072; 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=VX1/kosqWpxwBOx4VlQL+H4XvhWpo3gVcK51kC20+sM=; b=hgyVAk6YgnAPtUGajKbzElP66wS86ReR8/9pP5+JNVHFhps0p4UQwHnmdsJ6074CKi 1MVuYljF0h73vjuvytaUKm/YwThmf4XDPUIIAxNzAqlVdM6j8AryV/GrxmvHjoIKUUbJ SnqnrGOgpzOFNghVvqoiG7zvatnKmuR1HkCXPjD19OE/FB/w6ESJgzAvCl60itwkjpCU yXHvyWON3poZbSw4SRG+IzDlpiFfl+euvMJVzsnhCpZUYm+gofyA6ddtGCHxdI7lCJuB VJ4uUjnyrJ94v74EeDD0zl2ruGYz9MhbBk1EgYuoM+11HMksAwgRjaig7ugkrG7sHIB3 fKQg== X-Gm-Message-State: AAQBX9eTnWgvRqQIorq7fojLEN7/0FjWjK+IIQQl8j7Vpj/wuV4yO9YF WvQKdKnngy1DKpnzfZusgrw32hin2jvsDvFVhbcJGPj7 X-Google-Smtp-Source: AKy350ZsjFzyhvkkHbOhic060do/Qm/dFsUcgJqrWTEGVSrMObTSuD+LMXhmOi91VdGDj4Gypm9QFrpBwtT2Ar/Pycw= X-Received: by 2002:a2e:9842:0:b0:2a8:c89b:ce38 with SMTP id e2-20020a2e9842000000b002a8c89bce38mr2312472ljj.5.1682338071623; Mon, 24 Apr 2023 05:07:51 -0700 (PDT) MIME-Version: 1.0 References: <20230422220921.452264-1-apinski@marvell.com> <20230422220921.452264-3-apinski@marvell.com> In-Reply-To: <20230422220921.452264-3-apinski@marvell.com> From: Richard Biener Date: Mon, 24 Apr 2023 14:06:26 +0200 Message-ID: Subject: Re: [PATCH 2/6] PHIOPT: Cleanup tree_ssa_phiopt_worker code 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.2 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, Apr 23, 2023 at 12:13=E2=80=AFAM Andrew Pinski via Gcc-patches wrote: > > This patch cleans up tree_ssa_phiopt_worker by merging > common code. Making do_store_elim handled earlier. > Note this does not change any overall logic of the code, > just moves code around enough to be able to do this. > This will make it easier to move code around even more > and a few other fixes I have. > Plus I think all of the do_store_elim code really > should move to its own function as how much code is shared > is now obvious not much. Agreed. > OK? Bootstrapped and tested on x86_64-linux-gnu. OK. Thanks, Richard. > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (tree_ssa_phiopt_worker): Rearrange > code for better code readability. > --- > gcc/tree-ssa-phiopt.cc | 212 +++++++++++++++++++++-------------------- > 1 file changed, 107 insertions(+), 105 deletions(-) > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index 296ba646e62..05f19825ce9 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -175,10 +175,14 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do= _hoist_loads, bool early_p) > std::swap (bb1, bb2); > std::swap (e1, e2); > } > - else if (EDGE_SUCC (bb1, 0)->dest =3D=3D EDGE_SUCC (bb2, 0)->dest) > + else if (EDGE_SUCC (bb1, 0)->dest =3D=3D EDGE_SUCC (bb2, 0)->dest > + && single_succ_p (bb2)) > { > diamond_p =3D true; > e2 =3D EDGE_SUCC (bb2, 0); > + /* Make sure bb2 is just a fall through. */ > + if ((e2->flags & EDGE_FALLTHRU) =3D=3D 0) > + continue; > } > else > continue; > @@ -190,46 +194,23 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do= _hoist_loads, bool early_p) > || (e1->flags & EDGE_FALLTHRU) =3D=3D 0) > continue; > > - if (do_store_elim && diamond_p) > + if (do_store_elim) > { > - basic_block bb3 =3D e1->dest; > - > - if (!single_succ_p (bb2) > - || (e2->flags & EDGE_FALLTHRU) =3D=3D 0 > - || EDGE_COUNT (bb3->preds) !=3D 2) > - continue; > - if (cond_if_else_store_replacement (bb1, bb2, bb3)) > - cfgchanged =3D true; > - continue; > - } > - else if (do_hoist_loads && diamond_p) > - { > - basic_block bb3 =3D e1->dest; > - > - if (!FLOAT_TYPE_P (TREE_TYPE (gimple_cond_lhs (cond_stmt))) > - && single_succ_p (bb2) > - && single_pred_p (bb1) > - && single_pred_p (bb2) > - && EDGE_COUNT (bb->succs) =3D=3D 2 > - && EDGE_COUNT (bb3->preds) =3D=3D 2 > - /* If one edge or the other is dominant, a conditional move > - is likely to perform worse than the well-predicted branc= h. */ > - && !predictable_edge_p (EDGE_SUCC (bb, 0)) > - && !predictable_edge_p (EDGE_SUCC (bb, 1))) > - hoist_adjacent_loads (bb, bb1, bb2, bb3); > - continue; > - } > - > - if (diamond_p) > - { > - if (!single_pred_p (bb1) > - || !single_pred_p (bb2) > - || !single_succ_p (bb2)) > - continue; > - } > + if (diamond_p) > + { > + basic_block bb3 =3D e1->dest; > + > + /* Only handle sinking of store from 2 bbs only, > + The middle bbs don't need to come from the > + if always since we are sinking rather than > + hoisting. */ > + if (EDGE_COUNT (bb3->preds) !=3D 2) > + continue; > + if (cond_if_else_store_replacement (bb1, bb2, bb3)) > + cfgchanged =3D true; > + continue; > + } > > - if (do_store_elim && !diamond_p) > - { > /* Also make sure that bb1 only have one predecessor and that i= t > is bb. */ > if (!single_pred_p (bb1) > @@ -243,85 +224,106 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool d= o_hoist_loads, bool early_p) > continue; > if (cond_store_replacement (bb1, bb2, e1, e2, nontrap)) > cfgchanged =3D true; > + continue; > } > - else > + > + if (diamond_p) > { > - gimple_stmt_iterator gsi; > - bool candorest =3D true; > + basic_block bb3 =3D e1->dest; > + > + if (!single_pred_p (bb1) > + || !single_pred_p (bb2)) > + continue; > > - /* Check that we're looking for nested phis. */ > - basic_block merge =3D diamond_p ? EDGE_SUCC (bb2, 0)->dest : bb= 2; > - gimple_seq phis =3D phi_nodes (merge); > + if (do_hoist_loads > + && !FLOAT_TYPE_P (TREE_TYPE (gimple_cond_lhs (cond_stmt))) > + && EDGE_COUNT (bb->succs) =3D=3D 2 > + && EDGE_COUNT (bb3->preds) =3D=3D 2 > + /* If one edge or the other is dominant, a conditional move > + is likely to perform worse than the well-predicted branc= h. */ > + && !predictable_edge_p (EDGE_SUCC (bb, 0)) > + && !predictable_edge_p (EDGE_SUCC (bb, 1))) > + { > + hoist_adjacent_loads (bb, bb1, bb2, bb3); > + continue; > + } > + } > > - /* Value replacement can work with more than one PHI > - so try that first. */ > - if (!early_p && !diamond_p) > - for (gsi =3D gsi_start (phis); !gsi_end_p (gsi); gsi_next (&g= si)) > + gimple_stmt_iterator gsi; > + bool candorest =3D true; > + > + /* Check that we're looking for nested phis. */ > + basic_block merge =3D diamond_p ? EDGE_SUCC (bb2, 0)->dest : bb2; > + gimple_seq phis =3D phi_nodes (merge); > + > + /* Value replacement can work with more than one PHI > + so try that first. */ > + if (!early_p && !diamond_p) > + for (gsi =3D gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi)) > + { > + phi =3D as_a (gsi_stmt (gsi)); > + arg0 =3D gimple_phi_arg_def (phi, e1->dest_idx); > + arg1 =3D gimple_phi_arg_def (phi, e2->dest_idx); > + if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) =3D= =3D 2) > { > - phi =3D as_a (gsi_stmt (gsi)); > - arg0 =3D gimple_phi_arg_def (phi, e1->dest_idx); > - arg1 =3D gimple_phi_arg_def (phi, e2->dest_idx); > - if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) = =3D=3D 2) > - { > - candorest =3D false; > - cfgchanged =3D true; > - break; > - } > + candorest =3D false; > + cfgchanged =3D true; > + break; > } > + } > > - if (!candorest) > - continue; > + if (!candorest) > + continue; > > - phi =3D single_non_singleton_phi_for_edges (phis, e1, e2); > - if (!phi) > - continue; > + phi =3D single_non_singleton_phi_for_edges (phis, e1, e2); > + if (!phi) > + continue; > + > + arg0 =3D gimple_phi_arg_def (phi, e1->dest_idx); > + arg1 =3D gimple_phi_arg_def (phi, e2->dest_idx); > > + /* Something is wrong if we cannot find the arguments in the PHI > + node. */ > + gcc_assert (arg0 !=3D NULL_TREE && arg1 !=3D NULL_TREE); > + > + gphi *newphi; > + if (single_pred_p (bb1) > + && !diamond_p > + && (newphi =3D factor_out_conditional_conversion (e1, e2, phi, > + arg0, arg1, > + cond_stmt))) > + { > + phi =3D newphi; > + /* factor_out_conditional_conversion may create a new PHI in > + BB2 and eliminate an existing PHI in BB2. Recompute values > + that may be affected by that change. */ > arg0 =3D gimple_phi_arg_def (phi, e1->dest_idx); > arg1 =3D gimple_phi_arg_def (phi, e2->dest_idx); > - > - /* Something is wrong if we cannot find the arguments in the PH= I > - node. */ > gcc_assert (arg0 !=3D NULL_TREE && arg1 !=3D NULL_TREE); > - > - gphi *newphi; > - if (single_pred_p (bb1) > - && !diamond_p > - && (newphi =3D factor_out_conditional_conversion (e1, e2, p= hi, > - arg0, arg1, > - cond_stmt))= ) > - { > - phi =3D newphi; > - /* factor_out_conditional_conversion may create a new PHI i= n > - BB2 and eliminate an existing PHI in BB2. Recompute val= ues > - that may be affected by that change. */ > - arg0 =3D gimple_phi_arg_def (phi, e1->dest_idx); > - arg1 =3D gimple_phi_arg_def (phi, e2->dest_idx); > - gcc_assert (arg0 !=3D NULL_TREE && arg1 !=3D NULL_TREE); > - } > - > - /* Do the replacement of conditional if it can be done. */ > - if (!early_p > - && !diamond_p > - && two_value_replacement (bb, bb1, e2, phi, arg0, arg1)) > - cfgchanged =3D true; > - else if (!diamond_p > - && match_simplify_replacement (bb, bb1, e1, e2, phi, > - arg0, arg1, early_p)) > - cfgchanged =3D true; > - else if (!early_p > - && !diamond_p > - && single_pred_p (bb1) > - && cond_removal_in_builtin_zero_pattern (bb, bb1, e1, = e2, > - phi, arg0, ar= g1)) > - cfgchanged =3D true; > - else if (minmax_replacement (bb, bb1, bb2, e1, e2, phi, arg0, a= rg1, > - diamond_p)) > - cfgchanged =3D true; > - else if (single_pred_p (bb1) > - && !diamond_p > - && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, = arg1)) > - cfgchanged =3D true; > } > + > + /* Do the replacement of conditional if it can be done. */ > + if (!early_p > + && !diamond_p > + && two_value_replacement (bb, bb1, e2, phi, arg0, arg1)) > + cfgchanged =3D true; > + else if (!diamond_p > + && match_simplify_replacement (bb, bb1, e1, e2, phi, > + arg0, arg1, early_p)) > + cfgchanged =3D true; > + else if (!early_p > + && !diamond_p > + && single_pred_p (bb1) > + && cond_removal_in_builtin_zero_pattern (bb, bb1, e1, e2, > + phi, arg0, arg1)) > + cfgchanged =3D true; > + else if (minmax_replacement (bb, bb1, bb2, e1, e2, phi, arg0, arg1= , > + diamond_p)) > + cfgchanged =3D true; > + else if (single_pred_p (bb1) > + && !diamond_p > + && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1= )) > + cfgchanged =3D true; > } > > free (bb_order); > -- > 2.39.1 >