From patchwork Wed Jul 26 11:40:57 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roger Sayle X-Patchwork-Id: 1813107 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=nextmovesoftware.com header.i=@nextmovesoftware.com header.a=rsa-sha256 header.s=default header.b=sDpzpdvO; dkim-atps=neutral Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4R9sRj1Vq0z1yYc for ; Wed, 26 Jul 2023 21:41:21 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id ECF5F3857024 for ; Wed, 26 Jul 2023 11:41:18 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from server.nextmovesoftware.com (server.nextmovesoftware.com [162.254.253.69]) by sourceware.org (Postfix) with ESMTPS id 9A2C93858C54 for ; Wed, 26 Jul 2023 11:41:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9A2C93858C54 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=nextmovesoftware.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=nextmovesoftware.com DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=nextmovesoftware.com; s=default; h=Content-Type:MIME-Version:Message-ID: Date:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=MiwxTS822+WDzlgmKAmeJkncletdlMjFTw4QAyD3lVI=; b=sDpzpdvOVsHC12xj6CFl/GGIY2 ukahNjHIUJEfaFqQDxIyB0EmH8EYRqNwVhLDPp/2TdPJqQmtaJrFs21YYACKTivwkyK8Gs23Zq1Wm daImhfJc26dRqoIz55DmE2apuwPkHhX32YIgvlzBGKbxAepCnwodL1cYqndmRd1iMYXeA2lcv50tW /6AOejlvVye+w4SmjdrGX0iE31KlweE9lrfueeDTJV0nCdp0qOYvtc5XCGP1G8MkQUzkTEHiAdmRb z/jEnT7Iw3jRteUYi5HdCphxtf/6oBPbGlcLk9RwGjQVuZBq4l/gTsrpbHT+aykzJ7PH4MucFKqnI thRg71ew==; Received: from [185.62.158.67] (port=51412 helo=Dell) by server.nextmovesoftware.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1qOct3-0005b2-1x; Wed, 26 Jul 2023 07:41:01 -0400 From: "Roger Sayle" To: Cc: , Subject: [PATCH] PR rtl-optimization/110701: Fix SUBREG SET_DEST handling in combine. Date: Wed, 26 Jul 2023 12:40:57 +0100 Message-ID: <04e101d9bfb6$0dacecd0$2906c670$@nextmovesoftware.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Content-Language: en-gb Thread-Index: Adm/tKcD6RvBALuHRqKBV5nTtRYUgg== X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - server.nextmovesoftware.com X-AntiAbuse: Original Domain - gcc.gnu.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - nextmovesoftware.com X-Get-Message-Sender-Via: server.nextmovesoftware.com: authenticated_id: roger@nextmovesoftware.com X-Authenticated-Sender: server.nextmovesoftware.com: roger@nextmovesoftware.com X-Source: X-Source-Args: X-Source-Dir: X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" This patch is my proposed fix to PR rtl-optimization 110701, a latent bug in combine's record_dead_and_set_regs_1 exposed by recent improvements to simplify_subreg. The issue involves the handling of (normal) SUBREG SET_DESTs as in the instruction: (set (subreg:HI (reg:SI x) 0) (expr:HI y)) The semantics of this are that the bits specified by the SUBREG are set to the SET_SRC, y, and that the other bits of the SET_DEST are left/become undefined. To simplify explanation, we'll only consider lowpart SUBREGs (though in theory non-lowpart SUBREGS could be handled), and the fact that bits outside of the lowpart WORD retain their original values (treating these as undefined is a missed optimization rather than incorrect code bug, that only affects targets with less than 64-bit words). The bug is that combine simulates the behaviour of the above instruction, for calculating nonzero_bits and set_sign_bit_copies, in the function record_value_for_reg, by using the equivalent of: (set (reg:SI x) (subreg:SI (expr:HI y)) by calling gen_lowpart on the SET_SRC. Alas, the semantics of this revised instruction aren't always equivalent to the original. In the test case for PR110701, the original instruction (set (subreg:HI (reg:SI x), 0) (and:HI (subreg:HI (reg:SI y) 0) (const_int 340))) which (by definition) leaves the top bits of x undefined, is mistakenly considered to be equivalent to (set (reg:SI x) (and:SI (reg:SI y) (const_int 340))) where gen_lowpart's freedom to do anything with paradoxical SUBREG bits, has now cleared the high bits. The same bug also triggers when the SET_SRC is say (subreg:HI (reg:DI z)), where gen_lowpart transforms this into (subreg:SI (reg:DI z)) which defines bits 16-31 to be the same as bits 16-31 of z. The fix is that after calling record_value_for_reg, we need to mark the bits that should be undefined as undefined, in case gen_lowpart, which performs transforms appropriate for r-values, has changed the interpretation of the SUBREG when used as an l-value. This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and make -k check, both with and without --target_board=unix{-m32} with no new failures. Ok for mainline? I've a version of this patch that preserves the original bits outside of the lowpart WORD that can be submitted as a follow-up, but this is the piece that addresses the wrong code regression. 2023-07-26 Roger Sayle gcc/ChangeLog PR rtl-optimization/110701 * combine.cc (record_dead_and_set_regs_1): Split comment into pieces placed before the relevant clauses. When the SET_DEST is a partial_subreg_p, mark the bits outside of the updated portion of the destination as undefined. gcc/testsuite/ChangeLog PR rtl-optimization/110701 * gcc.target/i386/pr110701.c: New test case. Thanks in advance, Roger diff --git a/gcc/combine.cc b/gcc/combine.cc index 4bf867d..c5ebb78 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -13337,27 +13337,43 @@ record_dead_and_set_regs_1 (rtx dest, const_rtx setter, void *data) if (REG_P (dest)) { - /* If we are setting the whole register, we know its value. Otherwise - show that we don't know the value. We can handle a SUBREG if it's - the low part, but we must be careful with paradoxical SUBREGs on - RISC architectures because we cannot strip e.g. an extension around - a load and record the naked load since the RTL middle-end considers - that the upper bits are defined according to LOAD_EXTEND_OP. */ + /* If we are setting the whole register, we know its value. */ if (GET_CODE (setter) == SET && dest == SET_DEST (setter)) record_value_for_reg (dest, record_dead_insn, SET_SRC (setter)); + /* We can handle a SUBREG if it's the low part, but we must be + careful with paradoxical SUBREGs on RISC architectures because + we cannot strip e.g. an extension around a load and record the + naked load since the RTL middle-end considers that the upper bits + are defined according to LOAD_EXTEND_OP. */ else if (GET_CODE (setter) == SET && GET_CODE (SET_DEST (setter)) == SUBREG && SUBREG_REG (SET_DEST (setter)) == dest && known_le (GET_MODE_PRECISION (GET_MODE (dest)), BITS_PER_WORD) && subreg_lowpart_p (SET_DEST (setter))) - record_value_for_reg (dest, record_dead_insn, - WORD_REGISTER_OPERATIONS - && word_register_operation_p (SET_SRC (setter)) - && paradoxical_subreg_p (SET_DEST (setter)) - ? SET_SRC (setter) - : gen_lowpart (GET_MODE (dest), - SET_SRC (setter))); + { + if (WORD_REGISTER_OPERATIONS + && word_register_operation_p (SET_SRC (setter)) + && paradoxical_subreg_p (SET_DEST (setter))) + record_value_for_reg (dest, record_dead_insn, SET_SRC (setter)); + else if (!partial_subreg_p (SET_DEST (setter))) + record_value_for_reg (dest, record_dead_insn, + gen_lowpart (GET_MODE (dest), + SET_SRC (setter))); + else + { + record_value_for_reg (dest, record_dead_insn, + gen_lowpart (GET_MODE (dest), + SET_SRC (setter))); + + unsigned HOST_WIDE_INT mask; + reg_stat_type *rsp = ®_stat[REGNO (dest)]; + mask = GET_MODE_MASK (GET_MODE (SET_DEST (setter))); + rsp->last_set_nonzero_bits |= ~mask; + rsp->last_set_sign_bit_copies = 1; + } + } + /* Otherwise show that we don't know the value. */ else record_value_for_reg (dest, record_dead_insn, NULL_RTX); } diff --git a/gcc/testsuite/gcc.target/i386/pr110701.c b/gcc/testsuite/gcc.target/i386/pr110701.c new file mode 100644 index 0000000..3f2cea5 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr110701.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ +int a; +long b; +int *c = &a; +short d(short e, short f) { return e * f; } +void foo() { + *c = d(340, b >= 0) ^ 3; +} + +/* { dg-final { scan-assembler "andl\[ \\t]\\\$340," } } */ +/* { dg-final { scan-assembler-not "andw\[ \\t]\\\$340," } } */