From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x134.google.com (mail-lf1-x134.google.com [IPv6:2a00:1450:4864:20::134]) by sourceware.org (Postfix) with ESMTPS id 7F6C73852767 for ; Tue, 26 Sep 2023 11:41:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7F6C73852767 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-x134.google.com with SMTP id 2adb3069b0e04-5046bf37ec1so4321432e87.1 for ; Tue, 26 Sep 2023 04:41:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695728463; x=1696333263; 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=7+yV6tpAO92YXAnPCIXrrrpBLFcs1M1c3vmBF480JX8=; b=QcdXVpiWbxE1Ov9VcBIcG1AQnkNrA41OpFMnXUuS4yGqdxlM9dDRwpFFw2dpyf8s3N O5tTZHrjt6EoZLscAOEYqDB3ColATnmMOrGNL/SqMCgKHXpq0WmNszYUsUcviWK99NVJ bt8Lted/iZbjB/qnmTih0+DJZ8eXB6+ziEhio/0JlVeD5Beg+OX6DjQ2vQwl/ARduirt I4VoU+9r9zRVSMpAbKiauA690mXp84fu95IqVTsYouo6Oeemhbz0800XXPKogfy0VT9t vftnqqt4tGk++ahZcutGQrnZi2AyKrzcG3SxHI+kuX5PJoiYjvShca5OKj1KJosakJwp rAlg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695728463; x=1696333263; 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=7+yV6tpAO92YXAnPCIXrrrpBLFcs1M1c3vmBF480JX8=; b=Hkk3nbA6ZEiaB4d7z07KSxoq5b7tIEPM2ucyVWuVFvWca7VTN8SLE15zoOUz/G0Sec pN/UkPT0aaWIFkqvKXMv/pjWk/7ftrPnidiZm9AdUAug6iHx6ta5kC8mNRRtKTmzIQtT g8WSCL5Bg7itOOw1N/qiTWzplTm/vl9LFQ1c00Mxew5LcchqcYXce6wwtqBcKH71bb3u CDlViHM8zVQnrKkO/qOnzZpEsThWJpe/ABMmXrLNikoSNxvwWGNANlGvY2jWhvrsmKpc vNCewQJlSBKlIceZ+tVMDtzaMvkrb0tw1XZjMIBsa260Ik3D0kOtVJ5dRYw06aJytobk SEtg== X-Gm-Message-State: AOJu0YxWMOI1c55WBEqTO0tAWMMWzeH0HlgmADkC+7sDXTWIVjTaHNub 1WfcoDA+DT+DsKmoRmPpGixzktIKs8VGA9KKCiMjgAyH X-Google-Smtp-Source: AGHT+IHvro349rZ7+RdwtdtS2g0c5vqwUZmffIB1swhzg5saRCrLDx5JVCc/nQ+ytfWAlzbcZ/GPburIFZ3Ou8QVGBY= X-Received: by 2002:a05:6512:3ea:b0:4ff:aeef:b582 with SMTP id n10-20020a05651203ea00b004ffaeefb582mr7292397lfq.66.1695728462692; Tue, 26 Sep 2023 04:41:02 -0700 (PDT) MIME-Version: 1.0 References: <20230921080931.1954219-1-apinski@marvell.com> In-Reply-To: <20230921080931.1954219-1-apinski@marvell.com> From: Richard Biener Date: Tue, 26 Sep 2023 13:38:40 +0200 Message-ID: Subject: Re: [PATCH] PHIOPT: Fix minmax_replacement for three way 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.3 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 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 Thu, Sep 21, 2023 at 10:10=E2=80=AFAM Andrew Pinski wrote: > > So when diamond bb support was added to minmax_replacement in r13-1950-g9= bb19e143cfe, > the code was not expecting the alt_middle_bb not to exist if it was empty= (for threeway_p). > So when factor_out_conditional_conversion was used to factor out conversi= ons, it turns out > the assumption for alt_middle_bb to be wrong and we ended up with threewa= y_p being true but > having middle_bb being empty but alt_middle_bb not being empty which caus= es wrong code in > many cases. > > This patch fixes the issue by adding a test for the 2 cases where the ass= umption on > threeway_p case having the other bb being empty. > > Note my plan for GCC 15 is remove minmax_replacement as match.pd will cat= ch all cases > at that point. > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. OK. > PR tree-optimization/111469 > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (minmax_replacement): Fix > the assumption for the `non-diamond` handling cases > of diamond code. > > gcc/testsuite/ChangeLog: > > * gcc.c-torture/execute/pr111469-1.c: New test. > --- > .../gcc.c-torture/execute/pr111469-1.c | 38 +++++++++++++++++++ > gcc/tree-ssa-phiopt.cc | 10 ++++- > 2 files changed, 46 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr111469-1.c > > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr111469-1.c b/gcc/tests= uite/gcc.c-torture/execute/pr111469-1.c > new file mode 100644 > index 00000000000..b68d5989eac > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr111469-1.c > @@ -0,0 +1,38 @@ > +/* PR tree-optimization/111469 */ > + > +long f; > +char *g; > +__attribute__((noinline)) > +char o() { > + char l; > + while (f) > + ; > + l =3D *g; > + return l; > +} > + > +/* factor_out_conditional_conversion is able to remove the casts > + from the 2 bbs (correctly) > + but then minmax_replacement should not optimize this to a MIN_EXPR > + as o has side effects. */ > + > +__attribute__((noinline)) > +unsigned short gg(unsigned short a, unsigned short b) > +{ > + short d; > + if (a > b) > + { > + d=3D b; > + } > + else > + { > + o(); > + d =3D a; > + } > + return d; > +} > + > +int main(void) > +{ > + gg(3, 2); > +} > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index 3835d25d08c..96901a40444 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -1823,7 +1823,9 @@ minmax_replacement (basic_block cond_bb, basic_bloc= k middle_bb, basic_block alt_ > arg_false =3D arg0; > } > > - if (empty_block_p (middle_bb)) > + if (empty_block_p (middle_bb) > + && (!threeway_p > + || empty_block_p (alt_middle_bb))) > { > if ((operand_equal_for_phi_arg_p (arg_true, smaller) > || (alt_smaller > @@ -2006,7 +2008,9 @@ minmax_replacement (basic_block cond_bb, basic_bloc= k middle_bb, basic_block alt_ > > return true; > } > - else > + else if (middle_bb =3D=3D alt_middle_bb > + && (!threeway_p > + || empty_block_p (alt_middle_bb))) > { > /* Recognize the following case, assuming d <=3D u: > > @@ -2182,6 +2186,8 @@ minmax_replacement (basic_block cond_bb, basic_bloc= k middle_bb, basic_block alt_ > SSA_OP_DEF)); > gsi_move_before (&gsi_from, &gsi); > } > + else > + return false; > > /* Emit the statement to compute min/max. */ > gimple_seq stmts =3D NULL; > -- > 2.31.1 >