From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x531.google.com (mail-pg1-x531.google.com [IPv6:2607:f8b0:4864:20::531]) by sourceware.org (Postfix) with ESMTPS id 41BC93858D1E for ; Tue, 20 Jun 2023 02:16:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 41BC93858D1E 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-x531.google.com with SMTP id 41be03b00d2f7-553a998bca3so1856636a12.2 for ; Mon, 19 Jun 2023 19:16:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687227413; x=1689819413; 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=BVETG6tNApoOFdJ7b31HA5TP8xHefu7Rhdaf+pQvQbQ=; b=FzIhmdBB1vYwDe7Y77dwpsAzfaoxRv+/yQKeh5bv34VykBwd+6LYj+IIhblJqlcPwV Q0cXgAinDrtK2iQ3spsA03OvB9nqByQHf8giwGZ7sA2zccw/UM0/9HvzPMLTKgiU9uFe MdfsljJjBJIX21HpRvWp8vlnGy5uDjTSBGVksZBegaRk/pOsbL1ekayCMcc6xDRKlaiD lEKavb2tOYxmLnjQsg8bI+qj/099Q4rwn/z96YwhnECW/z9LhRTXcY7/BYacQtYEREZ2 VSmJyV22Psjx5JkVenIglSFMt5Swl7qZtovMJX/CuTpKEwwOGvLxCMNQ+4ZHl1sHL6uZ wQkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687227413; x=1689819413; 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=BVETG6tNApoOFdJ7b31HA5TP8xHefu7Rhdaf+pQvQbQ=; b=CF23oylf1PXr9eT74AtH4IxklWREeJxBfRXu9yswgdr8Mhew5BnYx+X9K7s92dKd5f 3oRUSJW3YOr6ycVJcfAlrOxaVXFAvlua9VnDFcDNb3zAPQmfOEKq8/TYrxhvQjh6k2wr ZXO47C0ew2cjX+97jIaITJQAcMWeVQUs0ZwM0pvs7ZqrarSi8MJ58zYhaXp6v77PeQ7l MwiY/F4ulQvyQPpmMpD1T2y/7daXYd4Q3Y43fX7o8p3lU/I849JDsTjLBZ7RQtat+DzA vE26M6yfnxY3df1CRzubWjHGCfegzkwU3SXD2Ro23ulu9+Q24jw/pQeioJOzzhJ9qgZr Gwlw== X-Gm-Message-State: AC+VfDy/lz2iWvvkvweRlSikh+aPTueq1xxq8Bt2zVk/Ly6SDrwaFOVV seOzODvQpy2XC1HwbUj0Cm4= X-Google-Smtp-Source: ACHHUZ7Ovaum/o1kWUQSrlfCvDEwMDQeHZEZYBMAKBNj53KUuUjdNHl4zZsbVciS1IIpd656k4jKFg== X-Received: by 2002:a05:6a21:168e:b0:111:97f:6d9d with SMTP id np14-20020a056a21168e00b00111097f6d9dmr11202995pzb.62.1687227412848; Mon, 19 Jun 2023 19:16:52 -0700 (PDT) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id a17-20020a62e211000000b0066887dc50easm281728pfi.3.2023.06.19.19.16.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 19 Jun 2023 19:16:52 -0700 (PDT) Message-ID: <3bdd7695-ad88-d8d9-5133-05cb95623949@gmail.com> Date: Mon, 19 Jun 2023 20:16:50 -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 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible. Content-Language: en-US To: Andrew Pinski , Thiago Jung Bauermann Cc: Manolis Tsamis , Philipp Tomsich , Richard Biener , Palmer Dabbelt , Kito Cheng , gcc-patches@gcc.gnu.org, Tamar Christina References: <20230525123550.1072506-1-manolis.tsamis@vrull.eu> <20230525123550.1072506-3-manolis.tsamis@vrull.eu> <5836d561-2986-484c-8d9a-744c948e8602@gmail.com> <87ttv3xud1.fsf@linaro.org> From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,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 6/19/23 17:48, Andrew Pinski wrote: > On Mon, Jun 19, 2023 at 4:40 PM Andrew Pinski wrote: >> >> On Mon, Jun 19, 2023 at 9:58 AM Thiago Jung Bauermann via Gcc-patches >> wrote: >>> >>> >>> Hello Manolis, >>> >>> Philipp Tomsich writes: >>> >>>> On Thu, 8 Jun 2023 at 00:18, Jeff Law wrote: >>>>> >>>>> On 5/25/23 06:35, Manolis Tsamis wrote: >>>>>> Propagation of the stack pointer in cprop_hardreg is currenty forbidden >>>>>> in all cases, due to maybe_mode_change returning NULL. Relax this >>>>>> restriction and allow propagation when no mode change is requested. >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> * regcprop.cc (maybe_mode_change): Enable stack pointer propagation. >>>>> Thanks for the clarification. This is OK for the trunk. It looks >>>>> generic enough to have value going forward now rather than waiting. >>>> >>>> Rebased, retested, and applied to trunk. Thanks! >>> >>> Our CI found a couple of tests that started failing on aarch64-linux >>> after this commit. I was able to confirm manually that they don't happen >>> in the commit immediately before this one, and also that these failures >>> are still present in today's trunk. >>> >>> I have testsuite logs for last good commit, first bad commit and current >>> trunk here: >>> >>> https://people.linaro.org/~thiago.bauermann/gcc-regression-6a2e8dcbbd4b/ >>> >>> Could you please check? >>> >>> These are the new failures: >>> >>> Running gcc:gcc.target/aarch64/aarch64.exp ... >>> FAIL: gcc.target/aarch64/stack-check-cfa-3.c scan-assembler-times mov\\tx11, sp 1 >> >> So for the above before this change we had: >> ``` >> (insn:TI 597 596 598 2 (set (reg:DI 11 x11) >> (reg/f:DI 31 sp)) "stack-check-prologue-16.c":16:1 65 {*movdi_aarch64} >> (nil)) >> (insn 598 597 599 2 (set (mem:BLK (scratch) [0 A8]) >> (unspec:BLK [ >> (reg:DI 11 x11) >> (reg/f:DI 31 sp) >> ] UNSPEC_PRLG_STK)) "stack-check-prologue-16.c":16:1 1169 >> {stack_tie} >> (expr_list:REG_DEAD (reg:DI 11 x11) >> (nil))) >> ``` >> >> After we get: >> ``` >> (insn 598 596 599 2 (set (mem:BLK (scratch) [0 A8]) >> (unspec:BLK [ >> (reg:DI 31 sp [11]) repeated x2 >> ] UNSPEC_PRLG_STK)) "stack-check-prologue-16.c":16:1 1169 >> {stack_tie} >> (nil)) >> ``` >> Which seems to be ok, except we still have: >> .cfi_def_cfa_register 11 >> >> That is because on: >> (insn/f 596 595 598 2 (set (reg:DI 12 x12) >> (plus:DI (reg:DI 12 x12) >> (const_int 272 [0x110]))) "stack-check-prologue-16.c":16:1 >> 153 {*adddi3_aarch64} >> (expr_list:REG_CFA_DEF_CFA (reg:DI 11 x11) >> (nil))) >> >> We record x11 but never update it though that came before the mov for >> x11 ... So it seems like cprop_hardreg had no idea it needed to update >> it. >> >> I suspect the other testcases are just propagation of sp into the >> stores and such and just needed update. But the above testcase seems >> getting broken cfi though I don't know how to fix it. > > The code from aarch64.cc: > ``` > /* This is done to provide unwinding information for the stack > adjustments we're about to do, however to prevent the optimizers > from removing the R11 move and leaving the CFA note (which would be > very wrong) we tie the old and new stack pointer together. > The tie will expand to nothing but the optimizers will not touch > the instruction. */ > rtx stack_ptr_copy = gen_rtx_REG (Pmode, STACK_CLASH_SVE_CFA_REGNUM); > emit_move_insn (stack_ptr_copy, stack_pointer_rtx); > emit_insn (gen_stack_tie (stack_ptr_copy, stack_pointer_rtx)); > > /* We want the CFA independent of the stack pointer for the > duration of the loop. */ > add_reg_note (insn, REG_CFA_DEF_CFA, stack_ptr_copy); > RTX_FRAME_RELATED_P (insn) = 1; > ``` > > Well except now with this change, the optimizers touch this > instruction. Maybe the move instruction should not be a move but an > unspec so optimizers don't know what the move was. > Adding Tamar to the CC who added this code to aarch64 originally for > comments on the above understanding here. It's a bit hackish, but could we reject the stack pointer for operand1 in the stack-tie? And if we do so, does it help? jeff