From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x432.google.com (mail-pf1-x432.google.com [IPv6:2607:f8b0:4864:20::432]) by sourceware.org (Postfix) with ESMTPS id C48A73858D32 for ; Wed, 19 Jul 2023 04:31:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C48A73858D32 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-pf1-x432.google.com with SMTP id d2e1a72fcca58-6687096c6ddso4207644b3a.0 for ; Tue, 18 Jul 2023 21:31:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689741077; x=1692333077; 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=0lo152YzmeLHuvtgCtfWdAd/v3PzO2JSnQQ1Y1o/PgQ=; b=RLMpfkOZ5cnq9STm2KX6EQlRPAy+J7cYNz/nHzqD0kJtudwjrjuZnawCs87dJDNe74 vyaHcnRM6xCgEtqgAej/hlSX6WXqIUF60L4xunKhMqg2l8yLG6ZEq7o0/0NYRAkVJev1 auWhpKD3kVxyE3zPdNWmpjuMZLYAF2grgDELnx1kSROHbB8YL1SlCWLzmERHWJ3rSL66 65fns71Rh6Rexg5T7eschkCLEuT8YgNrmGus+mISFH0E8bDD30fG/Q/5AOXS47FxHou6 FghTST1gXocwiPFhcnwv9hGG9TDs9232TrTVNbHhEvrfku92nZBr/+jvqK1sWoAzx9+6 ArIA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689741077; x=1692333077; 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=0lo152YzmeLHuvtgCtfWdAd/v3PzO2JSnQQ1Y1o/PgQ=; b=cnad43MTkiN8Bqfp8pNpNM+Y3HQYPzXpeP64eYqrZYcogG54iVuDoiAPj1kyxHers6 JfMJOCh4TEFbWm3+ZBfHKNmbB+ZVtDt0YERXxbtaHlcKf0cigbTfaCFE0YtFRFOVpFTp 5qMBL9VGW4UWbYfL1ChDledUsnmD/WFpNpNxdOsn+QLFeKOqNe5SK5o0+fTbLtQLQtkM aK+Kzc7lNA24QzPKHIwoJvnRxkEliAotLCPDYNAYRUyTxCY8qnixQ4ndcsLKkzWpTJ3N YkeJ2Soz9Qa2CxzUS3F++21bUv5obxvANUX03KuU3rQMwFwaMmccIKlG2vZ4KrlZCC6w NXRQ== X-Gm-Message-State: ABy/qLaDdEFZNFOzU79rE7jXS8iURxh+0WJZCv6eDwe66SNKQ4PrY208 vQes9/A032N9kUGSCVWqW0k= X-Google-Smtp-Source: APBJJlFic2igyR6yFOOk8aeOvbiG36410fX4RlkczwKXrKmtJb6P/oUyYLgs8KmF663qAY0mKMLq2A== X-Received: by 2002:a05:6a00:14d3:b0:662:f0d0:a77d with SMTP id w19-20020a056a0014d300b00662f0d0a77dmr17022825pfu.30.1689741077213; Tue, 18 Jul 2023 21:31:17 -0700 (PDT) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id a7-20020a62bd07000000b0067886c78745sm2261153pff.66.2023.07.18.21.31.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jul 2023 21:31:16 -0700 (PDT) Message-ID: <93fd5732-145a-94ab-5a9e-5a2eb8bac886@gmail.com> Date: Tue, 18 Jul 2023 22:31:15 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v3] Implement new RTL optimizations pass: fold-mem-offsets. Content-Language: en-US To: Vineet Gupta , Manolis Tsamis Cc: gcc-patches@gcc.gnu.org, Philipp Tomsich , Richard Biener , Jakub Jelinek , gnu-toolchain References: <20230713141336.3950751-1-manolis.tsamis@vrull.eu> <1420ed26-9c30-b09e-f45c-54c89516814e@gmail.com> <59f02830-eae9-6a52-dd63-48a7217905ef@rivosinc.com> From: Jeff Law In-Reply-To: <59f02830-eae9-6a52-dd63-48a7217905ef@rivosinc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=BAYES_00,BODY_8BITS,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,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 List-Id: On 7/18/23 17:42, Vineet Gupta wrote: > Hi Manolis, > > On 7/18/23 11:01, Jeff Law via Gcc-patches wrote: >> Vineet @ Rivos has indicated he stumbled across an ICE with the V3 >> code.  Hopefully he'll get a testcase for that extracted shortly. > > Yeah, I was trying to build SPEC2017 with this patch and ran into ICE > for several of them with -Ofast build: The reduced test from 455.nab is > attached here. > The issue happens with v2 as well, so not something introduced by v3. > > There's ICE in cprop_hardreg which immediately follows f-m-o. > > > The protagonist is ins 93 which starts off in combine as a simple set of > a DF 0. > > | sff.i.288r.combine:(insn 93 337 94 8 (set (reg/v:DF 236 [ e ]) > | sff.i.288r.combine- (const_double:DF 0.0 [0x0.0p+0])) "sff.i":23:11 > 190 {*movdf_hardfloat_rv64} > > Subsequently reload transforms it into SP + offset > > | sff.i.303r.reload:(insn 93 337 94 9 (set (mem/c:DF (plus:DI (reg/f:DI > 2 sp) > | sff.i.303r.reload- (const_int 8 [0x8])) [4 %sfp+-8 S8 A64]) > | sff.i.303r.reload- (const_double:DF 0.0 [0x0.0p+0])) "sff.i":23:11 190 > {*movdf_hardfloat_rv64} > | sff.i.303r.reload- (expr_list:REG_EQUAL (const_double:DF 0.0 [0x0.0p+0]) > > It gets processed by f-m-o and lands in cprop_hardreg, where it triggers > ICE. > > | (insn 93 337 523 11 (set (mem/c:DF (plus:DI (reg/f:DI 2 sp) > |                 (const_int 8 [0x8])) [4 %sfp+-8 S8 A64]) > |         (const_double:DF 0.0 [0x0.0p+0])) "sff.i":23:11 -1 > ^^^ > |      (expr_list:REG_EQUAL (const_double:DF 0.0 [0x0.0p+0]) > |        (nil))) > | during RTL pass: cprop_hardreg > > Here's my analysis: > > f-m-o: do_check_validity() -> insn_invalid_p() tries to recog() a > modified version of insn 93 (actually there is no change, so perhaps > something we can optimize later). The corresponding md pattern > movdf_hardfloat_rv64 no longer matches since it expects REG_P for > operand0, while reload has converted it into SP + offset. f-m-o then > does the right thing by invalidating INSN_CODE=-1 for a subsequent > recog() to work correctly. > But it seems this -1 lingers into the next pass, and trips up > copyprop_hardreg_forward_1() -> extract_constrain_insn() > So I don't know what the right fix here should be. This is a bug in the RISC-V backend. I actually fixed basically the same bug in another backend that was exposed by the f-m-o code. > > In a run with -fno-fold-mem-offsets, the same insn 93 is successfully > grok'ed by cprop_hardreg, > > | (insn 93 337 522 11 (set (mem/c:DF (plus:DI (reg/f:DI 2 sp) > |                (const_int 8 [0x8])) [4 %sfp+-8 S8 A64]) > |        (const_double:DF 0.0 [0x0.0p+0])) "sff.i":23:11 190 > {*movdf_hardfloat_rv64} > ^^^^^^^^^^^^^^^ > |     (expr_list:REG_EQUAL (const_double:DF 0.0 [0x0.0p+0]) > |        (nil))) > > P.S. I wonder if it is a good idea in general to call recog() post > reload since the insn could be changed sufficiently to no longer match > the md patterns. Of course I don't know the answer. If this ever causes a problem, it's a backend bug. It's that simple. Conceptually it should always be safe to set INSN_CODE to -1 for any insn. Odds are for this specific case in the RV backend, we just need a constraint to store 0.0 into a memory location. That can actually be implemented as a store from x0 since 0.0 has the bit pattern 0x0. This is probably a good thing to expose anyway as an optimization and can move forward independently of the f-m-o patch. > > P.S.2 When debugging code, I noticed a minor annoyance in the patch with > the whole fold_mem_offsets_driver() switch-case indirection. It doesn't > seem to be serving any purpose, and we could simply call corresponding > do_* routines in execute () itself. We were in the process of squashing some of this out of the implementation. I hadn't looked at the V3 patch to see how much progress had been made on this yet. Thanks for digging into this! jeff