From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62c.google.com (mail-pl1-x62c.google.com [IPv6:2607:f8b0:4864:20::62c]) by sourceware.org (Postfix) with ESMTPS id E1BAA3858D20 for ; Wed, 5 Apr 2023 13:14:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E1BAA3858D20 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-pl1-x62c.google.com with SMTP id kc4so34348859plb.10 for ; Wed, 05 Apr 2023 06:14:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1680700466; x=1683292466; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=6+H9GIB8H7G74BTa+oMLauM4dZBnM7qFC9yjGWZfIqo=; b=Uc7wcqRRsDDu/SNON5IFGXc4Q0CSwVL8NSEUJQyZXGPnkWmJFG5sNPyf50shTS9HI3 znv/irD9YNFaB1L5cv/czjOR62MqOLSvNsVBbVztFjuEZvqdg6K8vcCKE+nOC9f5DiYj NcJh8Y1Qxq8Zfir9RzliVfnjA3hlOVvs2xPbonXzHczLAnG+UPEfjqt8P2L2BrO/LWJQ 3G/EMO4TWGOD3qrWVCKpvZ+EeOfuiI31IZJjsh7GjrJgZXqqLKF6ySMvz/Xb2vgTi3hY XBjkXJjojuwMjjCyFToAbIdLxR/tdbWurg81CaCJ4P959JGiuMt1unPf+4y4ndJHt9nr DZXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680700466; x=1683292466; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=6+H9GIB8H7G74BTa+oMLauM4dZBnM7qFC9yjGWZfIqo=; b=J4gpqx8BSGmRxk65bzKDzc4aJ7WO+yVyh+51S1Jj+gazzWqOHEy+NYaTDEfTxzGz9R hJi+Uv3fqwc5VNnL/NdDvuSBswuNgU/GFfoEtMNU95Yi4dJ6uCitAiCBw4UO5Q1msBoR rhEp7QlsD0Rn15Xpe9psiyZ8PYs9TaJzWc2nPHJS/2z/3A3od8ZAR02cBEeOkpsd9GA9 DL61BbhQsnjoRN1NkMbIi2iT1rssX5+DwPga1yLnZjA1pSV1qfmB2IXEJo1TwiTcOwgc HYrF6ytDSmYYVbqD9dzpfdvpN1yDNySZewdxwarZ1bpi9JebOv70LlLecr2TOIETs3o2 FnLA== X-Gm-Message-State: AAQBX9cmMNSBYqBiKfMZXabc63w7JWTo2XTAE+t2HKgC1VdEUscdRlmw qqysDA91AkpRyqEkgdwRE8E= X-Google-Smtp-Source: AKy350Y41Csa6geXaFT0twgFeefmHPm+IoswncppRFvT8S/FKpdgDUJpdJqUQual/LUzAJZBvdxeBQ== X-Received: by 2002:a17:90b:4a91:b0:240:1d50:2725 with SMTP id lp17-20020a17090b4a9100b002401d502725mr6508974pjb.30.1680700465589; Wed, 05 Apr 2023 06:14:25 -0700 (PDT) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id q6-20020a170902dac600b0019c2cf12d15sm10111844plx.116.2023.04.05.06.14.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 05 Apr 2023 06:14:25 -0700 (PDT) Message-ID: <8e0e3cd5-e4db-ce8a-b7dc-baac32aed516@gmail.com> Date: Wed, 5 Apr 2023 07:14:23 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH] dse: Handle SUBREGs of word REGs differently for WORD_REGISTER_OPERATIONS targets [PR109040] Content-Language: en-US To: Jakub Jelinek , Richard Biener , Richard Sandiford , Eric Botcazou Cc: gcc-patches@gcc.gnu.org References: From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,NICE_REPLY_A,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 4/5/23 03:16, Jakub Jelinek wrote: > Hi! > > The following testcase is miscompiled on riscv since the addition > of *mvconst_internal define_insn_and_split. > I believe the bug is in DSE. We have: > (insn 36 35 39 2 (set (mem/c:SI (plus:SI (reg/f:SI 65 frame) > (const_int -64 [0xffffffffffffffc0])) [2 S4 A128]) > (reg:SI 166)) "pr109040.c":9:11 178 {*movsi_internal} > (expr_list:REG_DEAD (reg:SI 166) > (nil))) > (insn 39 36 40 2 (set (reg:SI 171) > (zero_extend:SI (mem/c:HI (plus:SI (reg/f:SI 65 frame) > (const_int -64 [0xffffffffffffffc0])) [0 S2 A128]))) "pr109040.c":9:11 111 {*zero_extendhisi2} > (nil)) > and RTL DSE's replace_read since r0-86337-g18b526e806ab6455 handles > even different modes like in the above case, and so it optimizes it into: > (insn 47 35 39 2 (set (reg:HI 175) > (subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 179 {*movhi_internal} > (expr_list:REG_DEAD (reg:SI 166) > (nil))) > (insn 39 47 40 2 (set (reg:SI 171) > (zero_extend:SI (reg:HI 175))) "pr109040.c":9:11 111 {*zero_extendhisi2} > (expr_list:REG_DEAD (reg:HI 175) > (nil))) > Pseudo 166 is result of AND with 0x8084c constant (forced into a register). Right. But do we agree that the two above are equivalent? If they are then changing DSE just papers over the combine issue downstream. > Combine attempts to combine the AND with the insn 47 above created by DSE, > and turns it because of WORD_REGISTER_OPERATIONS and its assumption that all > the subword operations are actually done on word mode into: > (set (subreg:SI (reg:HI 175) 0) > (and:SI (reg:SI 167 [ m ]) > (reg:SI 168))) > and later on the ZERO_EXTEND is thrown away. And isn't that where the bug really is. There's a patch from pan2.li in this exact space. That patch still doesn't look right to me, but it seems to be attacking this problem closer to the right place. > > The following patch changes DSE to instead emit > (insn 47 35 39 2 (set (reg:SI 175) > (reg:SI 166)) "pr109040.c":10:11 180 {*movsi_internal} > (expr_list:REG_DEAD (reg:SI 166) > (nil))) > (insn 39 47 40 2 (set (reg:SI 171) > (zero_extend:SI (subreg:HI (reg:SI 175) 0))) "pr109040.c":10:11 111 {*zero_extendhisi2} > (expr_list:REG_DEAD (reg:SI 175) > (nil))) > i.e. in the new insn copy whole reg rather than subword part of it where > we don't really know anything about the upper bits. > With this change, combine manages to do the right thing, optimies the > (unsigned) (unsigned short) (reg & 0x8084c) into > reg & 0x84c. This may still be desirable but it feels like papering over the problem to me. > > Bootstrapped/regtested on x86_64-linux and i686-linux (admittedly not > WORD_REGISTER_OPERATIONS targets) and tested using a cross to riscv > on the testcase. I have unfortunately no way to bootstrap this on > risc*-linux, could somebody do that? Ok for trunk if that passes/ > > 2023-04-05 Jakub Jelinek > > PR target/109040 > * dse.cc (replace_read): If read_reg is a SUBREG of a word mode > REG, for WORD_REGISTER_OPERATIONS copy SUBREG_REG of it into > a new REG rather than the SUBREG. > > * gcc.c-torture/execute/pr109040.c: New test. No problem with the patch itself. I just want to make sure we're fixing the problem in the right place. jeff