From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com [IPv6:2a00:1450:4864:20::12e]) by sourceware.org (Postfix) with ESMTPS id 6645D3857343 for ; Tue, 26 Sep 2023 11:40:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6645D3857343 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-x12e.google.com with SMTP id 2adb3069b0e04-503397ee920so13628091e87.1 for ; Tue, 26 Sep 2023 04:40:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695728436; x=1696333236; 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=eHv2A8nDApMCjCv4EVWxCceJbMUb2s0K2gKPN5//3v0=; b=anKJKo1JaKQ//zXUwD9vLNSRRKmtB7nNon/7eTfr/KjF3yDhKoc6FhR/KT1koCFger ngSodIwHzYFBiwQictsX2iLgZV7FFV785yQ91LPHWiXq4YbKcJpvSn1g5akyVJiEmHuH /mit7xH2cu28t/JE4gEbAHok/EUvPTZU+AvwvD3TE+5gPk/Z3m6eEV66QfqufRxnt6gn b+qz9faItVXIsvG7d5e4P3ZT8dH+v0OFmAOK6NbPf8wh4LIkw6alw8LazHg2qvUFixal Na0oTj0jP0eFeGlOXZ8m0eBQZt2Sakp2S1b5QZawpt1PaghsaVOV1oa7mepac/lDyhuQ mLWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695728436; x=1696333236; 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=eHv2A8nDApMCjCv4EVWxCceJbMUb2s0K2gKPN5//3v0=; b=nGgqFI8jLoCSuXWVJI+z2SWe9rsTItKrzCURFmg2+AISOfjpgxNThIl8FK1s0IgDDK rv7pTEHjv3UZ4rU65s10QXppAxxB/viYSnmcBRufm6XawP2G7wjy3eRsD/qrjoRa+k1e V0QYRGaw9cpUN8ySzfKFk5GXD1SFm3o3Q7VXXhUhNE88aPrGxwuuM90lciN795m1ZSxB 4vrJxYfgANjfONHq+IFkcwtV4ZpUDU8v+C4kveJnrSjfU5ChRFb79s2ODt08rP/UA3rB UEP47TymWecnkahP/dWjIHCYt0GTAiJiiT9N+9Z7s23qF0tkXWRnuYcLiSWNxG/UDomk GE0g== X-Gm-Message-State: AOJu0YxPNo3oHLrhWZP1WYjEiwasX5qNmf3UwOzHogxaH+NWbVS3yeqw 0ERPdwsY6JhJIHsMAdHqE6CNsSgWw8J9zIUr9NI= X-Google-Smtp-Source: AGHT+IH28CXNjf6jTn4UI20P84cRyCJsnJidGF8sn6clNOiOWEO54Gvlp+0wWtWPB/4+atj6VYfJ2PlI/P28ZHkxGWM= X-Received: by 2002:a05:6512:3490:b0:502:d5b0:436e with SMTP id v16-20020a056512349000b00502d5b0436emr6910071lfr.62.1695728435532; Tue, 26 Sep 2023 04:40:35 -0700 (PDT) MIME-Version: 1.0 References: <20230923005427.2053950-1-apinski@marvell.com> In-Reply-To: <20230923005427.2053950-1-apinski@marvell.com> From: Richard Biener Date: Tue, 26 Sep 2023 13:38:13 +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.1 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 Sat, Sep 23, 2023 at 2:55=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. > > Changes made: > v2: Fix test for `(a <=3D u) b =3D MAX(a, d) else b =3D u`. > > 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 | 9 ++++- > 2 files changed, 45 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..312a6f9082b 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,8 @@ minmax_replacement (basic_block cond_bb, basic_bloc= k middle_bb, basic_block alt_ > > return true; > } > - else > + else if (!threeway_p > + || empty_block_p (alt_middle_bb)) > { > /* Recognize the following case, assuming d <=3D u: > > @@ -2182,6 +2185,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 >