[committed,v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
Message ID | alpine.DEB.2.20.2111021546270.18331@tpp.orcam.me.uk |
---|---|
State | Committed |
Commit | c33a5cc9e7f1475108892abb147f9382ecbaec12 |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7B1B2385780A for <patchwork@sourceware.org>; Tue, 2 Nov 2021 16:07:02 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id 6F2FA3858015 for <gcc-patches@gcc.gnu.org>; Tue, 2 Nov 2021 16:06:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6F2FA3858015 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wr1-x42c.google.com with SMTP id d27so16134985wrb.6 for <gcc-patches@gcc.gnu.org>; Tue, 02 Nov 2021 09:06:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=hjnebakCOmTBK4wH0dD+aP1MC6vzNqv+lfuxLQ7J9oY=; b=E/svAkU+urNmzeA3dnuId6x6NPPFdEYzO+d0T4BB6AYO+I5Injla18E8BaWmzz/lQL 5LwZPTulkdu9q4vY1i+Y+KgeZlJiYqQqb7LgdROncNroOD1w5DTbsR0Iqnnj2SQSbQJq 5jwLgPQ0k4zFT808iRdotepllfbhM45j6317q08RZR2Cl6JJhPdugme1HDD/MAEGJm5F 0wI6Gu+O7bAFEyQDJpDnFTw/jMqouCY8TYDb9ijsmVghh20RjuaFtDIF4T5xhsme/uab xwlcDFwOdYHqwbMpUXkmifTAq8d97AjFZW/9Q0ZUa/XdlZtaUlGo+K1wj3ZVhHTpmkk6 nGHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=hjnebakCOmTBK4wH0dD+aP1MC6vzNqv+lfuxLQ7J9oY=; b=sBohPUV0idXHs0EzVWDcfNOrxXRpuURGH7mtsYXOjC3Xf1g6HCUB7YbvD+K4O+DiqD +vlOK2s8Pb6P3/Jnw2pgp1FTVLnb4GjY+XbFfvsSBdS5EDb09q4fjRdyrjXixONpRi7d QX7Hwv2vF7it680MgTUM4sY1GE7PUPyPkbbazgrruZhgx18Mo9DopKkqNsGJxcLcWcDQ 2hzMkM49yi3Rta3802yoGLWhqaZLViqP4CBQZ+wt6hG53WrBauFo+IeJpeI+2UWmw7or UBmDv7lcBtsQQ+BRSyjV0893c34VcTX80+4ao4IZzpw7LItftwH+CdBIUDgEvaX5YzJF MLIA== X-Gm-Message-State: AOAM530u8d3wQKVXbmQ36B92rbL9aMrjtqjncU+e/6n08qzY0SDysm5a 6jznXAtuE7vZLREFkEeNi8qcZA== X-Google-Smtp-Source: ABdhPJzLtK0NngPZlxRFDISgieikA4GDN1MERUPooWfzbARq48PTZLBKT9B50iyd/9UCPazvR/lTFA== X-Received: by 2002:adf:e482:: with SMTP id i2mr19973342wrm.284.1635869177503; Tue, 02 Nov 2021 09:06:17 -0700 (PDT) Received: from [192.168.0.201] ([212.69.42.53]) by smtp.gmail.com with ESMTPSA id n7sm15971778wro.68.2021.11.02.09.06.16 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Nov 2021 09:06:17 -0700 (PDT) Date: Tue, 2 Nov 2021 16:06:16 +0000 (GMT) From: "Maciej W. Rozycki" <macro@embecosm.com> To: Kito Cheng <kito.cheng@gmail.com> Subject: [committed v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model In-Reply-To: <CA+yXCZCRYXrvVPt0emnsroDMop2HEHMw2MBfsvF58Gk2avh5VA@mail.gmail.com> Message-ID: <alpine.DEB.2.20.2111021546270.18331@tpp.orcam.me.uk> References: <alpine.DEB.2.20.2111011214570.3741@tpp.orcam.me.uk> <CA+yXCZCRYXrvVPt0emnsroDMop2HEHMw2MBfsvF58Gk2avh5VA@mail.gmail.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Cc: GCC Patches <gcc-patches@gcc.gnu.org>, Andrew Waterman <andrew@sifive.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[committed,v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
|
|
Commit Message
Maciej W. Rozycki
Nov. 2, 2021, 4:06 p.m. UTC
Fix a build regression from commit 04a9b554ba1a ("RISC-V: Cost model for zba extension."): .../gcc/config/riscv/riscv.c: In function 'bool riscv_rtx_costs(rtx, machine_mode, int, int, int*, bool)': .../gcc/config/riscv/riscv.c:2018:11: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror] 2018 | && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3)) | ^~ .../gcc/config/riscv/riscv.c:2047:17: error: unused variable 'ashift_lhs' [-Werror=unused-variable] 2047 | rtx ashift_lhs = XEXP (and_lhs, 0); | ^~~~~~~~~~ by correcting a CONST_INT_P check referring the wrong operand and getting rid of the unused variable. gcc/ * config/riscv/riscv.c (riscv_rtx_costs): Correct a CONST_INT_P check and remove an unused local variable with shNadd/shNadd.uw pattern handling. --- Hi Kito, > I think that's my mistake...it should fix following check rather than > remove the REG_P like that: > > @@ -2014,8 +2014,8 @@ riscv_rtx_costs (rtx x, machine_mode mode, int > outer_code, int opno ATTRIBUTE_UN > (TARGET_64BIT && (mode == DImode))) > && (GET_CODE (XEXP (x, 0)) == ASHIFT) > && REG_P (XEXP (XEXP (x, 0), 0)) > - && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > - && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3)) > + && CONST_INT_P (XEXP (XEXP (x, 0), 1)) > + && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 1)), 1, 3)) > { > *total = COSTS_N_INSNS (1); > return true; > > > shNadd pattern: > > (define_insn "*shNadd" > [(set (match_operand:X 0 "register_operand" "=r") > (plus:X (ashift:X (match_operand:X 1 "register_operand" "r") > # What I want to check is here, it should be > XEXP (XEXP (x, 0), 1) > (match_operand:QI 2 "immediate_operand" "I")) > (match_operand:X 3 "register_operand" "r")))] Right, I should have cross-checked with the machine description. Also are we missing explicit test coverage here? Or is it supposed to be covered by the generic tests here or there already (I'm not familiar with the details of the ISA extension to tell offhand), as long as the extension has been enabled for the target tested, and it is just that the problem has slipped through somehow? > Otherwise LGTM, feel free to commit once you address this issue. Rebuilt for verification and committed as shown. Thank you for your review. Maciej --- gcc/config/riscv/riscv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) gcc-riscv-rtx-costs-zba-shnadd.diff
Comments
On Wed, Nov 3, 2021 at 12:07 AM Maciej W. Rozycki <macro@embecosm.com> wrote: > > Fix a build regression from commit 04a9b554ba1a ("RISC-V: Cost model > for zba extension."): > > .../gcc/config/riscv/riscv.c: In function 'bool riscv_rtx_costs(rtx, machine_mode, int, int, int*, bool)': > .../gcc/config/riscv/riscv.c:2018:11: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror] > 2018 | && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3)) > | ^~ > .../gcc/config/riscv/riscv.c:2047:17: error: unused variable 'ashift_lhs' [-Werror=unused-variable] > 2047 | rtx ashift_lhs = XEXP (and_lhs, 0); > | ^~~~~~~~~~ > > > by correcting a CONST_INT_P check referring the wrong operand and > getting rid of the unused variable. > > gcc/ > * config/riscv/riscv.c (riscv_rtx_costs): Correct a CONST_INT_P > check and remove an unused local variable with shNadd/shNadd.uw > pattern handling. > --- > Hi Kito, > > > I think that's my mistake...it should fix following check rather than > > remove the REG_P like that: > > > > @@ -2014,8 +2014,8 @@ riscv_rtx_costs (rtx x, machine_mode mode, int > > outer_code, int opno ATTRIBUTE_UN > > (TARGET_64BIT && (mode == DImode))) > > && (GET_CODE (XEXP (x, 0)) == ASHIFT) > > && REG_P (XEXP (XEXP (x, 0), 0)) > > - && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > > - && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3)) > > + && CONST_INT_P (XEXP (XEXP (x, 0), 1)) > > + && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 1)), 1, 3)) > > { > > *total = COSTS_N_INSNS (1); > > return true; > > > > > > shNadd pattern: > > > > (define_insn "*shNadd" > > [(set (match_operand:X 0 "register_operand" "=r") > > (plus:X (ashift:X (match_operand:X 1 "register_operand" "r") > > # What I want to check is here, it should be > > XEXP (XEXP (x, 0), 1) > > (match_operand:QI 2 "immediate_operand" "I")) > > (match_operand:X 3 "register_operand" "r")))] > > Right, I should have cross-checked with the machine description. > > Also are we missing explicit test coverage here? Or is it supposed to > be covered by the generic tests here or there already (I'm not familiar > with the details of the ISA extension to tell offhand), as long as the > extension has been enabled for the target tested, and it is just that > the problem has slipped through somehow? Cost model is not easy to test (at least to me :p), I usually verify that by check the dump of combine pass to make sure the cost is right. > > Otherwise LGTM, feel free to commit once you address this issue. > > Rebuilt for verification and committed as shown. Thank you for your > review. Thanks! > > Maciej > --- > gcc/config/riscv/riscv.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > gcc-riscv-rtx-costs-zba-shnadd.diff > Index: gcc/gcc/config/riscv/riscv.c > =================================================================== > --- gcc.orig/gcc/config/riscv/riscv.c > +++ gcc/gcc/config/riscv/riscv.c > @@ -2014,8 +2014,8 @@ riscv_rtx_costs (rtx x, machine_mode mod > (TARGET_64BIT && (mode == DImode))) > && (GET_CODE (XEXP (x, 0)) == ASHIFT) > && REG_P (XEXP (XEXP (x, 0), 0)) > - && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > - && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3)) > + && CONST_INT_P (XEXP (XEXP (x, 0), 1)) > + && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 1)), 1, 3)) > { > *total = COSTS_N_INSNS (1); > return true; > @@ -2044,7 +2044,6 @@ riscv_rtx_costs (rtx x, machine_mode mod > if (!CONST_INT_P (and_rhs)) > break; > > - rtx ashift_lhs = XEXP (and_lhs, 0); > rtx ashift_rhs = XEXP (and_lhs, 1); > > if (!CONST_INT_P (ashift_rhs)
On Wed, 3 Nov 2021, Kito Cheng wrote: > > Also are we missing explicit test coverage here? Or is it supposed to > > be covered by the generic tests here or there already (I'm not familiar > > with the details of the ISA extension to tell offhand), as long as the > > extension has been enabled for the target tested, and it is just that > > the problem has slipped through somehow? > > Cost model is not easy to test (at least to me :p), I usually verify > that by check the dump of combine pass to make sure the cost is right. Fair enough. Note that intermediate dumps can be used for automatic verification too, with `scan-tree-dump', `scan-rtl-dump', etc. (I guess you probably knew that already). I know that writing a good test case can be tricky. Maciej
Index: gcc/gcc/config/riscv/riscv.c =================================================================== --- gcc.orig/gcc/config/riscv/riscv.c +++ gcc/gcc/config/riscv/riscv.c @@ -2014,8 +2014,8 @@ riscv_rtx_costs (rtx x, machine_mode mod (TARGET_64BIT && (mode == DImode))) && (GET_CODE (XEXP (x, 0)) == ASHIFT) && REG_P (XEXP (XEXP (x, 0), 0)) - && CONST_INT_P (XEXP (XEXP (x, 0), 0)) - && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3)) + && CONST_INT_P (XEXP (XEXP (x, 0), 1)) + && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 1)), 1, 3)) { *total = COSTS_N_INSNS (1); return true; @@ -2044,7 +2044,6 @@ riscv_rtx_costs (rtx x, machine_mode mod if (!CONST_INT_P (and_rhs)) break; - rtx ashift_lhs = XEXP (and_lhs, 0); rtx ashift_rhs = XEXP (and_lhs, 1); if (!CONST_INT_P (ashift_rhs)