From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com [IPv6:2a00:1450:4864:20::32e]) by sourceware.org (Postfix) with ESMTPS id 7312D3847700 for ; Fri, 26 Jul 2024 08:31:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7312D3847700 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=vrull.eu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=vrull.eu ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7312D3847700 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::32e ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1721982675; cv=none; b=SFpkthvdKmkWie2jxI/Rn1sJFUBwKG1Mj0bDoeuAKZ1dewtxEcKR4z+WQgblpATP2BfQglzzAKSY+tsZ/q7j25fq/TmW155YLF2ssWnVjY3aw3AkaWfGfdtJRGn0AZEHBOVyUYXfuu6Q93KhWAsRei5pXoMVvfMJYNamBTyGa3k= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1721982675; c=relaxed/simple; bh=oB7ptHCp5iKEH5WAAGv9a+eSMyMqWLG5GhW4uP0mW4Q=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=LzfIGW+it0lD50jdUmFoG4MxKd/rYTAaNC2YkCUkfRGTkGzIQC/GCqpC9g9uAgoxvZuLgHZEJ+fKogQleICQ7b35Bg6a6kTOhA18wv740py3UyTwPKNsm3uZW0GMn8HypFUZKE4qEhVNVEhDkh0QCTZqp/cqk8k6wzU1ZSNV5vM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x32e.google.com with SMTP id 5b1f17b1804b1-4280812ca01so9999875e9.1 for ; Fri, 26 Jul 2024 01:31:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vrull.eu; s=google; t=1721982671; x=1722587471; darn=gcc.gnu.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=6MrfcUaTf3PUSxsQoliSaC0foOa/cmAcB69lcpDqY+o=; b=lFuhCOfj2wwaQ9Ku6KP58lX8UomwTn7v2ZJdeIBNhEBqEd9oyzxHX06mGJl31mdofp 9PYTKSxlww050/u1vNhsrnXfnhFee5t+Z47uCZ4GXWqnrQ4Yd3R7wNdZ2UInTtuZGOb0 Vf7AlE3uKKqmb8qeXwU8Q+jqtv6eu6SUwxEkMBqeMtTaEXatk4pM2AD/6/ruu/XeUtwj KMAJ0ZkD08Yuer3ynrCPl2zx5UhsF6h6PDSyPHP386TEGZ0b4kZZBa9VWr2OZ8V99FZ8 NdRs7Ex/odB7WkoOo+sRJVtUUIM+OodQ7Y9n3PFUaGP2HmYrY2WtSSC6W+XOT9k199xE 5Mmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721982671; x=1722587471; h=content-transfer-encoding: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=6MrfcUaTf3PUSxsQoliSaC0foOa/cmAcB69lcpDqY+o=; b=mdrveHT0kGt8I8HvjTqjChVbHeORrZrLNSykT9Zhp137m6vv+qSKuVRzMAGyInmT+a qxscHh2v4vz9L4PpvWC2VNvigsUZAYSFpULKOX+eSPCkzqjBNSdrXhpTK0SM0eWxLMfy ob2tQ6ZOajpBJcDDZP4rSoLqXo1zmCecX9e3rrdh47KUiRdFLA7fBHx8rvX9XynDxlxe 5wxb8KP3Vh0a8ncKLJd5klGMpP0o7rE7jyqJJoBuQoeusoOXlIO4u6BS80rZ09AKFrHr fBFmyeNAYMQm9a8B+WrX17zGRtwsWio3CbmPQK8gffoOKe7dj2jfLPTJ5Jf2hcmY8XVm AyAA== X-Forwarded-Encrypted: i=1; AJvYcCVGJkBSSyEOk4hMi/v0WCzS0d0QETEYZJQ04jdScbNQC5bh/+a1MytBTJtn2rD9etBoOqDfSBG5xZTfnhA2MOQhhIV1DcuFvg== X-Gm-Message-State: AOJu0YxMtw9Drndau+u9xRPdCjA3FmPABot355mWhiiZZtt5MwguK3HS fGbihdHSaLlyrEvEA3epNdnbuaLyM400B3Ei/IlZeji8S8o6ixn615eRwW7TXpFoteZ2/mzNGYj SfW+Ds9YJlhCHyXLjaKMa4D7e9v+9Osikrh4UYOl5PwDVScCjEn0= X-Google-Smtp-Source: AGHT+IEluWLcTQjHQpop/e3K8P5/esa4w9F1QLIYloE56lCHEdUjexpd4/4KGjd6z79Dk+2Kp91xr0ko51BXD73TP3M= X-Received: by 2002:a05:6000:1544:b0:368:557a:c64d with SMTP id ffacd0b85a97d-36b319d238dmr4985177f8f.9.1721982670740; Fri, 26 Jul 2024 01:31:10 -0700 (PDT) MIME-Version: 1.0 References: <20240423104740.4027243-1-manolis.tsamis@vrull.eu> <20240423104740.4027243-2-manolis.tsamis@vrull.eu> In-Reply-To: From: Manolis Tsamis Date: Fri, 26 Jul 2024 11:30:35 +0300 Message-ID: Subject: Re: [PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets To: Manolis Tsamis , gcc-patches@gcc.gnu.org, Robin Dapp , Jiangning Liu , Philipp Tomsich , Jakub Jelinek , richard.sandiford@arm.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_MANYTO,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 Wed, Jun 5, 2024 at 2:00=E2=80=AFPM Richard Sandiford wrote: > > Sorry for the slow review. > > Manolis Tsamis writes: > > This is an extension of what was done in PR106590. > > > > Currently if a sequence generated in noce_convert_multiple_sets clobber= s the > > condition rtx (cc_cmp or rev_cc_cmp) then only seq1 is used afterwards > > (sequences that emit the comparison itself). Since this applies only fr= om the > > next iteration it assumes that the sequences generated (in particular s= eq2) > > doesn't clobber the condition rtx itself before using it in the if_then= _else, > > which is only true in specific cases (currently only register/subregist= er moves > > are allowed). > > > > This patch changes this so it also tests if seq2 clobbers cc_cmp/rev_cc= _cmp in > > the current iteration. This makes it possible to include arithmetic ope= rations > > in noce_convert_multiple_sets. > > > > It also makes the code that checks whether the condition is used outsid= e of the > > if_then_else emitted more robust. > > > > gcc/ChangeLog: > > > > * ifcvt.cc (check_for_cc_cmp_clobbers): Use modified_in_p instead= . > > (noce_convert_multiple_sets_1): Don't use seq2 if it clobbers cc_= cmp. > > Refactor the code that sets read_comparison. > > > > Signed-off-by: Manolis Tsamis > > --- > > > > (no changes since v1) > > > > gcc/ifcvt.cc | 106 ++++++++++++++++++++++++++++----------------------- > > 1 file changed, 59 insertions(+), 47 deletions(-) > > > > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc > > index 58ed42673e5..763a25f816e 100644 > > --- a/gcc/ifcvt.cc > > +++ b/gcc/ifcvt.cc > > @@ -3592,20 +3592,6 @@ noce_convert_multiple_sets (struct noce_if_info = *if_info) > > return true; > > } > > > > -/* Helper function for noce_convert_multiple_sets_1. If store to > > - DEST can affect P[0] or P[1], clear P[0]. Called via note_stores. = */ > > - > > -static void > > -check_for_cc_cmp_clobbers (rtx dest, const_rtx, void *p0) > > -{ > > - rtx *p =3D (rtx *) p0; > > - if (p[0] =3D=3D NULL_RTX) > > - return; > > - if (reg_overlap_mentioned_p (dest, p[0]) > > - || (p[1] && reg_overlap_mentioned_p (dest, p[1]))) > > - p[0] =3D NULL_RTX; > > -} > > - > > /* This goes through all relevant insns of IF_INFO->then_bb and tries = to > > create conditional moves. In case a simple move sufficis the insn > > should be listed in NEED_NO_CMOV. The rewired-src cases should be > > @@ -3731,36 +3717,67 @@ noce_convert_multiple_sets_1 (struct noce_if_in= fo *if_info, > > creating an additional compare for each. If successful, costing > > is easier and this sequence is usually preferred. */ > > if (cc_cmp) > > - seq2 =3D try_emit_cmove_seq (if_info, temp, cond, > > - new_val, old_val, need_cmov, > > - &cost2, &temp_dest2, cc_cmp, rev_cc_cm= p); > > + { > > + seq2 =3D try_emit_cmove_seq (if_info, temp, cond, > > + new_val, old_val, need_cmov, > > + &cost2, &temp_dest2, cc_cmp, rev_cc_= cmp); > > + > > + /* The if_then_else in SEQ2 may be affected when cc_cmp/rev_cc_= cmp is > > + clobbered. We can't safely use the sequence in this case. = */ > > + if (seq2 && (modified_in_p (cc_cmp, seq2) > > + || (rev_cc_cmp && modified_in_p (rev_cc_cmp, seq2)))) > > + seq2 =3D NULL; > > It looks like this still has the problem that I mentioned in the > previous round: that modified_in_p only checks the first instruction > in seq2, not the whole sequence. Or is that the intention? > Fixed in v5. Thanks, Manolis > Thanks, > Richard > > > + } > > > > /* The backend might have created a sequence that uses the > > - condition. Check this. */ > > + condition as a value. Check this. */ > > + > > + /* We cannot handle anything more complex than a reg or constant= . */ > > + if (!REG_P (XEXP (cond, 0)) && !CONSTANT_P (XEXP (cond, 0))) > > + read_comparison =3D true; > > + > > + if (!REG_P (XEXP (cond, 1)) && !CONSTANT_P (XEXP (cond, 1))) > > + read_comparison =3D true; > > + > > rtx_insn *walk =3D seq2; > > - while (walk) > > + int if_then_else_count =3D 0; > > + while (walk && !read_comparison) > > { > > - rtx set =3D single_set (walk); > > + rtx exprs_to_check[2]; > > + unsigned int exprs_count =3D 0; > > > > - if (!set || !SET_SRC (set)) > > + rtx set =3D single_set (walk); > > + if (set && XEXP (set, 1) > > + && GET_CODE (XEXP (set, 1)) =3D=3D IF_THEN_ELSE) > > { > > - walk =3D NEXT_INSN (walk); > > - continue; > > + /* We assume that this is the cmove created by the backend = that > > + naturally uses the condition. */ > > + exprs_to_check[exprs_count++] =3D XEXP (XEXP (set, 1), 1); > > + exprs_to_check[exprs_count++] =3D XEXP (XEXP (set, 1), 2); > > + if_then_else_count++; > > } > > + else if (NONDEBUG_INSN_P (walk)) > > + exprs_to_check[exprs_count++] =3D PATTERN (walk); > > > > - rtx src =3D SET_SRC (set); > > + /* Bail if we get more than one if_then_else because the assump= tion > > + above may be incorrect. */ > > + if (if_then_else_count > 1) > > + { > > + read_comparison =3D true; > > + break; > > + } > > > > - if (XEXP (set, 1) && GET_CODE (XEXP (set, 1)) =3D=3D IF_THEN_EL= SE) > > - ; /* We assume that this is the cmove created by the backend = that > > - naturally uses the condition. Therefore we ignore it. = */ > > - else > > + for (unsigned int i =3D 0; i < exprs_count; i++) > > { > > - if (reg_mentioned_p (XEXP (cond, 0), src) > > - || reg_mentioned_p (XEXP (cond, 1), src)) > > - { > > - read_comparison =3D true; > > - break; > > - } > > + subrtx_iterator::array_type array; > > + FOR_EACH_SUBRTX (iter, array, exprs_to_check[i], NONCONST) > > + if (*iter !=3D NULL_RTX > > + && (reg_overlap_mentioned_p (XEXP (cond, 0), *iter) > > + || reg_overlap_mentioned_p (XEXP (cond, 1), *iter))) > > + { > > + read_comparison =3D true; > > + break; > > + } > > } > > > > walk =3D NEXT_INSN (walk); > > @@ -3788,21 +3805,16 @@ noce_convert_multiple_sets_1 (struct noce_if_in= fo *if_info, > > return false; > > } > > > > - if (cc_cmp) > > + if (cc_cmp && seq =3D=3D seq1) > > { > > - /* Check if SEQ can clobber registers mentioned in > > - cc_cmp and/or rev_cc_cmp. If yes, we need to use > > - only seq1 from that point on. */ > > - rtx cc_cmp_pair[2] =3D { cc_cmp, rev_cc_cmp }; > > - for (walk =3D seq; walk; walk =3D NEXT_INSN (walk)) > > + /* Check if SEQ can clobber registers mentioned in cc_cmp/rev_c= c_cmp. > > + If yes, we need to use only seq1 from that point on. > > + Only check when we use seq1 since we have already tested seq= 2. */ > > + if (modified_in_p (cc_cmp, seq) > > + || (rev_cc_cmp && modified_in_p (rev_cc_cmp, seq))) > > { > > - note_stores (walk, check_for_cc_cmp_clobbers, cc_cmp_pair); > > - if (cc_cmp_pair[0] =3D=3D NULL_RTX) > > - { > > - cc_cmp =3D NULL_RTX; > > - rev_cc_cmp =3D NULL_RTX; > > - break; > > - } > > + cc_cmp =3D NULL_RTX; > > + rev_cc_cmp =3D NULL_RTX; > > } > > }