From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) by sourceware.org (Postfix) with ESMTPS id B189A3858410 for ; Mon, 7 Nov 2022 12:56:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B189A3858410 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-ej1-x631.google.com with SMTP id sc25so29742436ejc.12 for ; Mon, 07 Nov 2022 04:56:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=XB2FIAsmRDt8LR+gaMX4TbtybR6x5zqXAivFh8ncasI=; b=Q8LBtE90apNgwptInrV0tM5igDlc8EfehGJ5Tk7VDOAffGAJckU0ppIOsoqnLrQlnf QbW+fT41w33mgO8NZybRAqDoGgoBQmzbGT+rinWpO3OMm02EnA4JaT6RVe3SaDBaqP3p yIhpLSTe0gDIlPBScRxdcUeaXN2z6dMxSDkk0PthtRF9SVSryy2y/ykXcfiRHGdGPEe8 IvNAVsH3Yh+30nJK6/g9w478gVEX6VBJ72iAsxnCqbN7LPHZQmwaSHlaQ1LsPPEL2bxA c7Qw+vIntTlN7NZ/n1MYavFkkioGSbOZitv/172yjC8iToazv88J0DwZur9IaDMaP3p7 7pGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=XB2FIAsmRDt8LR+gaMX4TbtybR6x5zqXAivFh8ncasI=; b=k0+Wbv+XMMbVGK0XcSAB0cDDmDTPVYoy01wlSSv6n7VLccconlpsgC06sEL1evNJmN NS0NKUqVQ20283gyjny6uFV8P8At/U2ymoADIbg2dpBsHiDceB7P6ZItV2CpudZHKhaR SYI7OydY/A5oaqnrFeoSNW5DOoE7GSBjSC4OaQxgQp34cjn0dWoX/Dl2xsiRkVIjb3cO T2L2VAtDFKbofXBUjYK0yK8UCDT+FS1vNSQLTaMfTCZc6mDWssRB01Gh7jDknFDHA73e Sv+pXinNijE3EnKP+rTifFa7SYJMbHVjhQpF8qrLc9BG/yWd0mz3xuzSfeg0ApZ8CPb0 x1tQ== X-Gm-Message-State: ACrzQf2kfCuIzRGUwLEBGrGL9eK+38FKEYmeKoDq1UZ6GV+WR243+cmk QDnLJfU8FIT6PEWIs7nauhAG/rHPOokpNrH0UR8= X-Google-Smtp-Source: AMsMyM7yLBS0VXD5lknQo2kgNmg4/Uaa4Kxu9XzLA1we6fsyeJMdIihmWLZm6NyQ8gHMvIYP+1Aqg+WGp/ql6s9COkM= X-Received: by 2002:a17:907:971f:b0:7ad:e232:f115 with SMTP id jg31-20020a170907971f00b007ade232f115mr35911778ejc.754.1667825813282; Mon, 07 Nov 2022 04:56:53 -0800 (PST) MIME-Version: 1.0 References: <20221021135203.626255-1-dimitrije.milosevic@syrmia.com> <20221021135203.626255-3-dimitrije.milosevic@syrmia.com> In-Reply-To: From: Richard Biener Date: Mon, 7 Nov 2022 13:56:41 +0100 Message-ID: Subject: Re: [PATCH 2/2] ivopts: Consider number of invariants when calculating register pressure. To: Dimitrije Milosevic Cc: "gcc-patches@gcc.gnu.org" , Djordje Todorovic Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.2 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 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 Fri, Oct 28, 2022 at 3:39 PM Dimitrije Milosevic wrote: > > Hi Richard, > > > It's n_invs + 2 * n_cands? > > Correct, n_invs + 2 * n_cands, my apologies. > > > The comment says we want to prefer eliminating IVs over invariants. Yo= ur patch > > undoes that by weighting invariants the same so it does no longer have > > the effect > > of the comment. > > I see how my patch may have confused you. > My concern is the "If we have enough registers." case - if we do have > enough registers to store both the invariants and induction variables, I = think the cost > should be equal to the sum of those. > > I understand that adding another n_cands could be used as a tie-breaker f= or the two > cases where we do have enough registers and the sum of n_invs and n_cands= is equal, > however I think there are two problems with that: > - How often does it happen that we have two cases where we do have enough= registers, > n_invs + n_cands sums are equal, and n_cands differ? I think that's pre= tty rare. > - Bumping up the cost by another n_cands may lead to cost for the "If we = do have > enough registers." case to be higher than for other cases, which doesn't = make sense. > I can refer to the test case that I presented in [0] for the second point= . The odd thing I notice is that for all cases but the "we have enough registers" case we scale cost by target_reg_cost[speed] or target_spill_cost[speed] but in that special case we use the plain sum of n_invs + n_cands. That makes this case "extra cheap" which is possibly OK. Instead of adding another n_invs it would have been clearer to remove the a= dd of n_cands, no? Or indeed as you say return n_new from the first case. > Also worth noting is that the estimate_reg_pressure_cost function (used b= efore c18101f) > follows this: > > /* If we have enough registers, we should use them and not restrict > the transformations unnecessarily. */ > if (regs_needed + target_res_regs <=3D available_regs) > return 0; > > As far as preferring to eliminate induction variables if possible, don't = we already do that, > for example: > > /* If the number of candidates runs out available registers, we penaliz= e > extra candidate registers using target_spill_cost * 2. Because it i= s > more expensive to spill induction variable than invariant. */ > else > cost =3D target_reg_cost [speed] * available_regs > + target_spill_cost [speed] * (n_cands - available_regs) * 2 > + target_spill_cost [speed] * (regs_needed - n_cands); > > To clarify, what my patch did was that it gave every case a base cost of > n_invs + n_cands. This base cost gets bumped up accordingly, for each > one of the cases (by the amount equal to "cost =3D ..." statement prior t= o > the return statement in the ivopts_estimate_reg_pressure function). > I agree that my patch isn't clear on my intention, and that it also does > not correspond to the comment. > What I could do is just return n_new as the cost for the > "If we do have enough registers." case, but I would love to hear your > thoughts, if I clarified my intention a little bit. You did. Note IVOPTs costing is tricky at best and I don't have a very goo= d feeling why biasing things with n_invs should be a general improvement. The situation where it makes a difference should be as rare as the situation you describe above. I'd love to see the function a bit simplified, it seems we are going to com= pare 'cost' as computed by this function from different cases so making them less apples vs. oranges would be good - I just don't see how your patch does that. It might be that the bias should be applied when computing iv_ca_delta or when comparing two iv_ca rather than trying to magically adjust 'cost'. When that is really the problem you are trying to solve. > > [0] https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604304.html > > Regards, > Dimitrije > > From: Richard Biener > Sent: Friday, October 28, 2022 9:38 AM > To: Dimitrije Milosevic > Cc: gcc-patches@gcc.gnu.org ; Djordje Todorovic = > Subject: Re: [PATCH 2/2] ivopts: Consider number of invariants when calcu= lating register pressure. > > On Tue, Oct 25, 2022 at 3:00 PM Dimitrije Milosevic > wrote: > > > > Hi Richard, > > > > > don't you add n_invs twice now given > > > > > > unsigned n_old =3D data->regs_used, n_new =3D n_invs + n_cands; > > > unsigned regs_needed =3D n_new + n_old, available_regs =3D target_av= ail_regs; > > > > > > ? > > > > If you are referring to the "If we have enough registers." case, correc= t. After c18101f, > > for that case, the returned cost is equal to 2 * n_invs + n_cands. > > It's n_invs + 2 * n_cands? And the comment states the reasoning. > > Before c18101f, for > > that case, the returned cost is equal to n_invs + n_cands. Another solu= tion would be > > to just return n_invs + n_cands if we have enough registers. > > The comment says we want to prefer eliminating IVs over invariants. Your= patch > undoes that by weighting invariants the same so it does no longer have > the effect > of the comment. > > > Regards, > > Dimitrije > > > > > > From: Richard Biener > > Sent: Tuesday, October 25, 2022 1:07 PM > > To: Dimitrije Milosevic > > Cc: gcc-patches@gcc.gnu.org ; Djordje Todorovi= c > > Subject: Re: [PATCH 2/2] ivopts: Consider number of invariants when cal= culating register pressure. > > > > On Fri, Oct 21, 2022 at 3:57 PM Dimitrije Milosevic > > wrote: > > > > > > From: Dimitrije Milo=C5=A1evi=C4=87 > > > > > > This patch slightly modifies register pressure model function to cons= ider > > > both the number of invariants and the number of candidates, rather th= an > > > just the number of candidates. This used to be the case before c18101= f. > > > > don't you add n_invs twice now given > > > > unsigned n_old =3D data->regs_used, n_new =3D n_invs + n_cands; > > unsigned regs_needed =3D n_new + n_old, available_regs =3D target_ava= il_regs; > > > > ? > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-loop-ivopts.cc (ivopts_estimate_reg_pressure): Adj= ust. > > > > > > Signed-off-by: Dimitrije Milosevic > > > --- > > > gcc/tree-ssa-loop-ivopts.cc | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.c= c > > > index d53ba05a4f6..9d0b669d671 100644 > > > --- a/gcc/tree-ssa-loop-ivopts.cc > > > +++ b/gcc/tree-ssa-loop-ivopts.cc > > > @@ -6409,9 +6409,9 @@ ivopts_estimate_reg_pressure (struct ivopts_dat= a *data, unsigned n_invs, > > > + target_spill_cost [speed] * (n_cands - available_regs) *= 2 > > > + target_spill_cost [speed] * (regs_needed - n_cands); > > > > > > - /* Finally, add the number of candidates, so that we prefer elimin= ating > > > - induction variables if possible. */ > > > - return cost + n_cands; > > > + /* Finally, add the number of invariants and the number of candida= tes, > > > + so that we prefer eliminating induction variables if possible. = */ > > > + return cost + n_invs + n_cands; > > > } > > > > > > /* For each size of the induction variable set determine the penalty= . */ > > > -- > > > 2.25.1 > > >