From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1029.google.com (mail-pj1-x1029.google.com [IPv6:2607:f8b0:4864:20::1029]) by sourceware.org (Postfix) with ESMTPS id 5727C3858D33 for ; Mon, 21 Aug 2023 21:00:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5727C3858D33 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-pj1-x1029.google.com with SMTP id 98e67ed59e1d1-26f7f71b9a7so516997a91.0 for ; Mon, 21 Aug 2023 14:00:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1692651622; x=1693256422; 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=QdI+68qDkDZmqsw8pfcPKe4OOHdsS3JLk3INBY4oWbI=; b=XtTvZUDghnfljIv5b7kdjIjMaCaWfMTK0x1P3CwlkJd6xlcW/Qfzcm8AoQR/t3Rd0W wZWC4OrcwHsrMqhkEq6DGXnX23RKRjp6DGVNzGDPB8PFMJFEoqTDQEA1m+qIkJoClQSG uEb42tY32SRq124ragqFhQAbdfFluIcwy8x9SThPXmdEaX2Dn2rM3myiJDMhW9tZThMF GLi3ymNgF20i0vh0IkeFhMKokolr7/zvqLWBsIy6q4L4qNor8D0rsqn5rdNrb6oJeJLl 47dAp1PBjJ0N9/zRY4aCTtnciS8VfdXIayI0GUXcOwzx6A/FcPwWhGebN/8Xmi2ja6UG xUFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692651622; x=1693256422; 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=QdI+68qDkDZmqsw8pfcPKe4OOHdsS3JLk3INBY4oWbI=; b=SdJYj8piRc+4/KTt9xigVMYsM39SrR7jDbZ+SbylzEMOr0eDFFBDOgVzVesfvw/c7G PLIWtTMshj3QzwcImFP40/KaX8dRsBbvFFS/6Fgc8+o/3gwA2Ewk5rFWKitvIonqCbCf hzEaB0769JieZ5HSoULZsuV9pcNBg8D4n8ysLQ7Qfz9Fqq7psl+DWt7QNNS91Uz5PY05 sDHxtAvvT67LyIX2ePp9KIrLaDXX8VIwEi8DdIOeE8Z7mROpkEftPcmZ1Lg7M/YesdMq tIf23knJG8KZLa1dRcPcPvwGBtOkg5gPsJtSXW0H3HXjbAG/Fw1LnU0ijhbZll2GVM/T RDTA== X-Gm-Message-State: AOJu0YzlbAbczamBnFtcTvgWAUTppqsHsmRI3P2eUO2BiqkyTGvL8O2l ZlmCwn3p+II9V9OJ6K1V7VBz7+RL29R2sFNSggg= X-Google-Smtp-Source: AGHT+IEBMoPIB8in0RkMwBs1vt/6+LyivISP9KArJh/kVF4A3QAImYYbNrOO8xhphnIrOY4ExVTFC62WVXCnOBfwkO8= X-Received: by 2002:a17:90b:374f:b0:260:d40f:6ade with SMTP id ne15-20020a17090b374f00b00260d40f6ademr6896538pjb.15.1692651621830; Mon, 21 Aug 2023 14:00:21 -0700 (PDT) MIME-Version: 1.0 References: <20230811091551.2758227-1-apinski@marvell.com> <20230811091551.2758227-2-apinski@marvell.com> <5b67902f-fcf4-8024-079d-0bb7ea6fda56@redhat.com> In-Reply-To: <5b67902f-fcf4-8024-079d-0bb7ea6fda56@redhat.com> From: Andrew Pinski Date: Mon, 21 Aug 2023 14:00:09 -0700 Message-ID: Subject: Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler To: Andrew MacLeod Cc: Richard Biener , Andrew Pinski , Aldy Hernandez , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.9 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 Fri, Aug 11, 2023 at 8:08=E2=80=AFAM Andrew MacLeod via Gcc-patches wrote: > > > On 8/11/23 05:51, Richard Biener wrote: > > On Fri, Aug 11, 2023 at 11:17=E2=80=AFAM Andrew Pinski via Gcc-patches > > wrote: > >> So it turns out there was a simplier way of starting to > >> improve VRP to start to fix PR 110131, PR 108360, and PR 108397. > >> That was rewrite test_for_singularity to use range_op_handler > >> and Value_Range. > >> > >> This patch implements that and > >> > >> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > I'm hoping Andrew/Aldy can have a look here. > > > > Richard. > > > >> gcc/ChangeLog: > >> > >> * vr-values.cc (test_for_singularity): Add edge argument > >> and rewrite using range_op_handler. > >> (simplify_compare_using_range_pairs): Use Value_Range > >> instead of value_range and update test_for_singularity call. > >> > >> gcc/testsuite/ChangeLog: > >> > >> * gcc.dg/tree-ssa/vrp124.c: New test. > >> * gcc.dg/tree-ssa/vrp125.c: New test. > >> --- > >> gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 +++++++++++++ > >> gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 +++++++++++++ > >> gcc/vr-values.cc | 91 ++++++++----------------= -- > >> 3 files changed, 114 insertions(+), 65 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c > >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c > >> > >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gc= c.dg/tree-ssa/vrp124.c > >> new file mode 100644 > >> index 00000000000..6ccbda35d1b > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c > >> @@ -0,0 +1,44 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-O2 -fdump-tree-optimized" } */ > >> + > >> +/* Should be optimized to a =3D=3D -100 */ > >> +int g(int a) > >> +{ > >> + if (a =3D=3D -100 || a >=3D 0) > >> + ; > >> + else > >> + return 0; > >> + return a < 0; > >> +} > >> + > >> +/* Should optimize to a =3D=3D 0 */ > >> +int f(int a) > >> +{ > >> + if (a =3D=3D 0 || a > 100) > >> + ; > >> + else > >> + return 0; > >> + return a < 50; > >> +} > >> + > >> +/* Should be optimized to a =3D=3D 0. */ > >> +int f2(int a) > >> +{ > >> + if (a =3D=3D 0 || a > 100) > >> + ; > >> + else > >> + return 0; > >> + return a < 100; > >> +} > >> + > >> +/* Should optimize to a =3D=3D 100 */ > >> +int f1(int a) > >> +{ > >> + if (a < 0 || a =3D=3D 100) > >> + ; > >> + else > >> + return 0; > >> + return a > 50; > >> +} > >> + > >> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */ > >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gc= c.dg/tree-ssa/vrp125.c > >> new file mode 100644 > >> index 00000000000..f6c2f8e35f1 > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c > >> @@ -0,0 +1,44 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-O2 -fdump-tree-optimized" } */ > >> + > >> +/* Should be optimized to a =3D=3D -100 */ > >> +int g(int a) > >> +{ > >> + if (a =3D=3D -100 || a =3D=3D -50 || a >=3D 0) > >> + ; > >> + else > >> + return 0; > >> + return a < -50; > >> +} > >> + > >> +/* Should optimize to a =3D=3D 0 */ > >> +int f(int a) > >> +{ > >> + if (a =3D=3D 0 || a =3D=3D 50 || a > 100) > >> + ; > >> + else > >> + return 0; > >> + return a < 50; > >> +} > >> + > >> +/* Should be optimized to a =3D=3D 0. */ > >> +int f2(int a) > >> +{ > >> + if (a =3D=3D 0 || a =3D=3D 50 || a > 100) > >> + ; > >> + else > >> + return 0; > >> + return a < 25; > >> +} > >> + > >> +/* Should optimize to a =3D=3D 100 */ > >> +int f1(int a) > >> +{ > >> + if (a < 0 || a =3D=3D 50 || a =3D=3D 100) > >> + ; > >> + else > >> + return 0; > >> + return a > 50; > >> +} > >> + > >> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */ > >> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc > >> index a4fddd62841..7004b0224bd 100644 > >> --- a/gcc/vr-values.cc > >> +++ b/gcc/vr-values.cc > >> @@ -907,66 +907,30 @@ simplify_using_ranges::simplify_bit_ops_using_ra= nges > >> a known value range VR. > >> > >> If there is one and only one value which will satisfy the > >> - conditional, then return that value. Else return NULL. > >> - > >> - If signed overflow must be undefined for the value to satisfy > >> - the conditional, then set *STRICT_OVERFLOW_P to true. */ > >> + conditional on the EDGE, then return that value. > >> + Else return NULL. */ > >> > >> static tree > >> test_for_singularity (enum tree_code cond_code, tree op0, > >> - tree op1, const value_range *vr) > >> + tree op1, Value_Range vr, bool edge) > > VR should be a "vrange &". THis is the top level base class for all > ranges of all types/kinds, and what we usually pass values around as if > we want tohem to be any kind. If this is inetger only, we'd pass a an > 'irange &' > > Value_Range is the opposite. Its the sink that contains one of each kind > of range and can switch around between them as needed. You do not want > to pass that by value! The generic engine uses these so it can suppose > floats. int, pointers, whatever... > > >> { > >> - tree min =3D NULL; > >> - tree max =3D NULL; > >> - > >> - /* Extract minimum/maximum values which satisfy the conditional as = it was > >> - written. */ > >> - if (cond_code =3D=3D LE_EXPR || cond_code =3D=3D LT_EXPR) > >> + /* This is already a singularity. */ > >> + if (cond_code =3D=3D NE_EXPR || cond_code =3D=3D EQ_EXPR) > >> + return NULL; > >> + auto range_op =3D range_op_handler (cond_code); > >> + int_range<2> op1_range (TREE_TYPE (op0)); > >> + wide_int w =3D wi::to_wide (op1); > >> + op1_range.set (TREE_TYPE (op1), w, w); > > If this is only going to work with integers, you might want to check > that somewhere or switch to irange and int_range_max.. > > You can make it work with any kind (if you know op1 is a constant) by > simply doing > > Value_Range op1_range (TREE_TYPE (op1)) > get_global_range_query->range_of_expr (op1_range, op1) > > That will convert trees to a the appropriate range... THis is also true > for integer constants... but you can also just do the WI conversion like > you do. > > The routine also get confusing to read because it passes in op0 and > op1, but of course ranger uses op1 and op2 nomenclature, and it looks a > bit confusing :-P I'd change the operands passed in to op1 and op2 if > we are rewriting the routine. > > >> + Value_Range vr1(TREE_TYPE (op0)); > >> + if (range_op.op1_range (vr1, TREE_TYPE (op0), > >> + edge ? range_true () : range_false (), > >> + op1_range)) > > IF you decide to stick with integers, then you can just make vr1 an > int_range_max vr1; > You don't need the type in the declaration if you know its an irange. > Value_Range needs a type because it needs to know if its an integr or a > floating point (or whatever) that it is going ot be used as. > >> { > >> - min =3D TYPE_MIN_VALUE (TREE_TYPE (op0)); > >> - > >> - max =3D op1; > >> - if (cond_code =3D=3D LT_EXPR) > >> - { > >> - tree one =3D build_int_cst (TREE_TYPE (op0), 1); > >> - max =3D fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one); > >> - /* Signal to compare_values_warnv this expr doesn't overflow= . */ > >> - if (EXPR_P (max)) > >> - suppress_warning (max, OPT_Woverflow); > >> - } > >> - } > >> - else if (cond_code =3D=3D GE_EXPR || cond_code =3D=3D GT_EXPR) > >> - { > >> - max =3D TYPE_MAX_VALUE (TREE_TYPE (op0)); > >> - > >> - min =3D op1; > >> - if (cond_code =3D=3D GT_EXPR) > >> - { > >> - tree one =3D build_int_cst (TREE_TYPE (op0), 1); > >> - min =3D fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one); > >> - /* Signal to compare_values_warnv this expr doesn't overflow= . */ > >> - if (EXPR_P (min)) > >> - suppress_warning (min, OPT_Woverflow); > >> - } > >> - } > >> - > >> - /* Now refine the minimum and maximum values using any > >> - value range information we have for op0. */ > >> - if (min && max) > >> - { > >> - tree type =3D TREE_TYPE (op0); > >> - tree tmin =3D wide_int_to_tree (type, vr->lower_bound ()); > >> - tree tmax =3D wide_int_to_tree (type, vr->upper_bound ()); > >> - if (compare_values (tmin, min) =3D=3D 1) > >> - min =3D tmin; > >> - if (compare_values (tmax, max) =3D=3D -1) > >> - max =3D tmax; > >> - > >> - /* If the new min/max values have converged to a single value, > >> - then there is only one value which can satisfy the condition, > >> - return that value. */ > >> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (m= in)) > >> - return min; > >> + vr.intersect (vr1); > >> + tree newop1; > >> + /* If the updated range is just a singleton, then we can just d= o a comparison */ > >> + if (vr.singleton_p (&newop1)) > >> + return newop1; > >> } > >> return NULL; > >> } > >> @@ -1224,9 +1188,9 @@ simplify_using_ranges::simplify_compare_using_ra= nges_1 (tree_code &cond_code, tr > >> && cond_code !=3D EQ_EXPR > >> && TREE_CODE (op0) =3D=3D SSA_NAME > >> && INTEGRAL_TYPE_P (TREE_TYPE (op0)) > >> - && is_gimple_min_invariant (op1)) > >> + && TREE_CODE (op1) =3D=3D INTEGER_CST) > >> { > >> - value_range vr; > >> + Value_Range vr (TREE_TYPE (op0)); > OK, so we know they are integers.. you could just iuse int_range_max > vr; if you want to stick to integers > >> > >> if (!query->range_of_expr (vr, op0, stmt)) > >> vr.set_undefined (); > >> @@ -1235,20 +1199,17 @@ simplify_using_ranges::simplify_compare_using_= ranges_1 (tree_code &cond_code, tr > >> able to simplify this conditional. */ > >> if (!vr.undefined_p () && !vr.varying_p ()) > >> { > >> - tree new_tree =3D test_for_singularity (cond_code, op0, op1,= &vr); > >> + tree new_tree =3D test_for_singularity (cond_code, op0, op1,= vr, > >> + true); > >> if (new_tree) > >> { > >> cond_code =3D EQ_EXPR; > >> op1 =3D new_tree; > >> happened =3D true; > >> } > >> - > >> - /* Try again after inverting the condition. We only deal > >> - with integral types here, so no need to worry about > >> - issues with inverting FP comparisons. */ > >> - new_tree =3D test_for_singularity > >> - (invert_tree_comparison (cond_code, false), > >> - op0, op1, &vr); > >> + /* Try again after inverting the condition. */ > >> + new_tree =3D test_for_singularity (cond_code, op0, op1, vr, > >> + false); > >> if (new_tree) > >> { > >> cond_code =3D NE_EXPR; > >> -- > >> 2.31.1 > >> > Other than that, LGTM. I am going to update this patch based on the review here; I just need a few more days to do it. Thanks, Andrew > > Andrew >