From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x532.google.com (mail-pg1-x532.google.com [IPv6:2607:f8b0:4864:20::532]) by sourceware.org (Postfix) with ESMTPS id 76F78385DC18 for ; Tue, 5 Sep 2023 07:46:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 76F78385DC18 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-pg1-x532.google.com with SMTP id 41be03b00d2f7-53fa455cd94so1160282a12.2 for ; Tue, 05 Sep 2023 00:46:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693899972; x=1694504772; 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=ltlNexmfh2Zsge/dYYaOYnfvmxc7DSXfnbJriK6h8tA=; b=P0aVz5C/Vy24YR+eBSe/PRA1F58sLrmXsSY+YDS+puJllIartWLGdSaLINgi/rxjrc XmepZVW6v6KiBrsuIDMQVTnWqH7/QPCvNi6sJC4+2GwxYOSZlNEQuKgX1JXa3Z7teljj W3zRYTIdYKPB8d95nyN6Wz/DsVRTbktY+DS5bkTsvvad8TDa9h/9Nzg3F+eCxvlcsfuP vBrlAPMchDyFnzvYnGJ0lGRZxQW06KZoOVvoUFlw6ZfdXprxZYLZB+H0kVg1v2ejp/yH uatMTi6gN1wy+bRNI2K+3XSkAIcVnsiizItiIBAKuhkYq5IxLK+wJ1/chxVamtyHtUJq JF+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693899972; x=1694504772; 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=ltlNexmfh2Zsge/dYYaOYnfvmxc7DSXfnbJriK6h8tA=; b=Gb1+r+D4AgckARly+pOJnMUIrPZE1CxcIMQbBmIWlNvS+6MxwxPXmfMerXayajLEQi cKzzuKiLU54Mncq1Z2qlLoRsIBssqjI2KFi+tS1D/kAOpVgeUdZ00eSaWdyAKvq2ZWwy m99C5pMl/TA/aZodBRApy0JWm56E7Kf3gZUVYpy0ACiyi78CSBdweUS2l61QXwwIyf+o Sib2j4LGdREBIpgxQmtGJtfCDkp4yqAt8L/Y1vBK8qYZBYGWCabt4+KMn5uSL92LDUdk +Irpy7BKddZ6dz1iio1NdP7sB1QJAD1CBUSJzKHptuB/IO1MeddWK2xU3xDB6vacJfcF r+6w== X-Gm-Message-State: AOJu0YwCwzq2+9TmmhXA/0bfa1QHdD6m4nZBVpxgS7FX8rHL8Olv9sXA oAmY7kxFdihTLnFhIzMrnnMQQAMWJJexbdl5uUU= X-Google-Smtp-Source: AGHT+IEtEHLbaAwZb7TwHHgBvIi1I27LpLIbTx2eXFxZcA40dlYTCOXLXDNkHwGE/iuipO9tA4yH7mZUtshd5Jg7Sd8= X-Received: by 2002:a17:90a:1548:b0:26d:54de:b0d6 with SMTP id y8-20020a17090a154800b0026d54deb0d6mr9152566pja.20.1693899972319; Tue, 05 Sep 2023 00:46:12 -0700 (PDT) MIME-Version: 1.0 References: <20230902023238.814338-1-apinski@marvell.com> <5a847448-1ef1-4101-a5fe-fe0236454ad0@gmail.com> In-Reply-To: <5a847448-1ef1-4101-a5fe-fe0236454ad0@gmail.com> From: Andrew Pinski Date: Tue, 5 Sep 2023 00:46:00 -0700 Message-ID: Subject: Re: [PATCH] ssa_name_has_boolean_range vs signed-boolean:31 types To: Jeff Law Cc: Andrew Pinski , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,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 Tue, Sep 5, 2023 at 12:09=E2=80=AFAM Jeff Law via Gcc-patches wrote: > > > > On 9/1/23 20:32, Andrew Pinski via Gcc-patches wrote: > > This turns out to be a latent bug in ssa_name_has_boolean_range > > where it would return true for all boolean types but all of the > > uses of ssa_name_has_boolean_range was expecting 0/1 as the range > > rather than [-1,0]. > > So when I fixed vector lower to do all comparisons in boolean_type > > rather than still in the signed-boolean:31 type (to fix a different iss= ue), > > the pattern in match for `-(type)!A -> (type)A - 1.` would assume A (wh= ich > > was signed-boolean:31) had a range of [0,1] which broke down and someti= mes > > gave us -1/-2 as values rather than what we were expecting of -1/0. > > > > This was the simpliest patch I found while testing. > > > > We have another way of matching [0,1] range which we could use instead > > of ssa_name_has_boolean_range except that uses only the global ranges > > rather than the local range (during VRP). > > I tried to clean this up slightly by using gimple_match_zero_one_valued= p > > inside ssa_name_has_boolean_range but that failed because due to using > > only the global ranges. I then tried to change get_nonzero_bits to use > > the local ranges at the optimization time but that failed also because > > we would remove branches to __builtin_unreachable during evrp and lose > > information as we don't set the global ranges during evrp. > > > > OK? Bootstrapped and tested on x86_64-linux-gnu. > > > > PR 110817 > > > > gcc/ChangeLog: > > > > * tree-ssanames.cc (ssa_name_has_boolean_range): Remove the > > check for boolean type as they don't have "[0,1]" range. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.c-torture/execute/pr110817-1.c: New test. > > * gcc.c-torture/execute/pr110817-2.c: New test. > > * gcc.c-torture/execute/pr110817-3.c: New test. > I'm a bit surprised this didn't trigger any regressions. Though maybe > all the existing testcases were capturing cases where non-boolean types > were known to have a 0/1 value. Well except ssa_name_has_boolean_range will return true for `An [unsigned] integral type with a single bit of precision` which the normal boolean type for C is. So the only case where this makes a difference is signed booleans. Vectors and Ada are the only 2 places I know of which use signed booleans even. This came up before too; https://inbox.sourceware.org/gcc-patches/CAFiYyc23ZMEvy6i9g1WpmPP7purcUzatG= 1QpwF2D_8n6F22QHQ@mail.gmail.com/ . Anyways the 3 uses of ssa_name_has_boolean_range in match.pd are: /* X / bool_range_Y is X. */ which is not true for signed booleans; though division for boolean types is not well defined /* 1 - a is a ^ 1 if a had a bool range. */ Which is broken for signed booleans; though it might not show up in IR for non 1-bit boolean types. /* -(type)!A -> (type)A - 1. */ This one 100 % requires `A` and `A =3D=3D 0` to be [0,1] range. The other uses of ssa_name_has_boolean_range are in DOM. The first 2 uses of ssa_name_has_boolean_range use build_one_cst/build_one_cst which is definitely wrong there. should have been constant_boolean_node for N-bit signed boolean types. The use `A COND_EXPR may create equivalences too.` actually does the correct thing and uses constant_boolean_node. Now maybe we miss some optimizations with Ada code with this change; I am not 100% sure. Maybe the change should just add && TYPE_UNSIGNED (type) to the check of boolean type and that will fix the issue too. > > > OK. > jeff