From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x631.google.com (mail-pl1-x631.google.com [IPv6:2607:f8b0:4864:20::631]) by sourceware.org (Postfix) with ESMTPS id 751DF385AC11 for ; Wed, 2 Nov 2022 15:02:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 751DF385AC11 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ventanamicro.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ventanamicro.com Received: by mail-pl1-x631.google.com with SMTP id u6so16818907plq.12 for ; Wed, 02 Nov 2022 08:02:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; 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=bBzJpZlbgqraEH5lkaz0hnXiQ8eWoakUDQJi2twAJTU=; b=EWji4LqQxFlHSt9Ml6HF87qs0w1h0wTV3/3InKMFQMbQZG1jk12ufjiqyyzdKEDEqo sBuTulju0SN4AV74b3hZrRkxHvHNj3R2gJ2xog7EyFCjwDp62HXANZSwLET9O7nEUPqj 90qC2IPm2ZsPlWwB5kJav82nO0/gmwRr7H5fSHVxm2Y8QNsapQLe9DK3jVDfFB5R1eQy inoMmUA+R3qOkLLg68732rKfjPvAT2q2bwRmdWvyyI30twjcT38pH7SFK37dzM3hJENS sRjIcs1BbL46tuz2A2+OlJoTXHZBMBtwdIVxwlQzdixufb7pg98K/s6evrAxpkzBLAgq A/Dw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=bBzJpZlbgqraEH5lkaz0hnXiQ8eWoakUDQJi2twAJTU=; b=NImW6nGZnrZOnVvQCQ11Jv+gnLSXre4LVWQHAcHzfet9Fv62JSKHsLfjRFoftG8C/E /ln0ocr6Zz4Hy2y66oRab+87aJQPfjnJEJ3GJ7ZkYfGD/F4Ivy74iFN4IpNQ8PxtcQuJ adRjEXtthCf2VwRBvZ1llLg6suMUOQ7PiSQB9a8ycTTysDLUteWV1XXKYvjpza8kkwra GBICZO0GEm9ZLxzsWMUhajllkZ7fR37+/3fLFlo7FWYJA4j7ggOOKD7zJKo5kVa9xBg2 bT5T9cYpJNkVvDwOfhawDeuoNEROkuYo4RMQAyl7+khb0zoJpweLeiOSnpEQb53ijL1a EjZA== X-Gm-Message-State: ACrzQf0405dDziCAM68Sgd1U7zh2Xa6unadvoHy3o/5hx2gxdAF1P1gg m2HSag+AsCxWTDMpst+KyUnsHA== X-Google-Smtp-Source: AMsMyM7ZGbuNvII/AlDw1qoBqlYfnLGh97evRvpG2AOrYyvH6lMo0t/lbVerLeK7A/8ppUJC+RdrFQ== X-Received: by 2002:a17:90b:28d:b0:214:650:e6fb with SMTP id az13-20020a17090b028d00b002140650e6fbmr10845812pjb.4.1667401373456; Wed, 02 Nov 2022 08:02:53 -0700 (PDT) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id x14-20020a62860e000000b0056c349f5c73sm8572488pfd.132.2022.11.02.08.02.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 02 Nov 2022 08:02:52 -0700 (PDT) Message-ID: <2cebf19f-d465-73f3-0334-523a79d5b306@ventanamicro.com> Date: Wed, 2 Nov 2022 09:02:51 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH] Enable shrink wrapping for the RISC-V target. Content-Language: en-US To: Manolis Tsamis , Jeff Law Cc: Palmer Dabbelt , gcc-patches@gcc.gnu.org, Vineet Gupta , Kito Cheng , philipp.tomsich@vrull.eu References: <08273c28-07ff-3170-679b-225d60a9ee2d@gmail.com> From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 11/2/22 08:12, Manolis Tsamis wrote: > On Wed, Oct 19, 2022 at 8:16 PM Jeff Law via Gcc-patches > wrote: >> >> On 10/18/22 11:35, Palmer Dabbelt wrote: >>>> I would have expected things to work fine with libcalls, perhaps with >>>> the exception of the save/restore libcalls. So that needs deeper >>>> investigation. >>> The save/restore libcalls only support saving/restoring a handful of >>> register configurations (just the saved X registers in the order >>> they're usually saved in by GCC). It should be OK for correctness to >>> over-save registers, but it kind of just un-does the shrink wrapping >>> so not sure it's worth worrying about at that point. >>> >>> There's also some oddness around the save/restore libcall ABI, it's >>> not the standard function ABI but instead a GCC-internal one. IIRC it >>> just uses the alternate link register (ie, t0 instead of ra) but I may >>> have forgotten something else. >> I hadn't really dug into it -- I was pretty sure they weren't following >> the standard ABI based on its name and how I've used similar routines to >> save space on some targets in the past. So if we're having problems >> with shrink-wrapping and libcalls, those two might be worth investigating. >> >> >> But I think the most important takeaway is that shrink wrapping should >> work with libcalls, there's nothing radically different about libcalls >> that would make them inherently interact poorly with shrink-wrapping. >> So that aspect of the shrink-wrapping patch needs deeper investigation. >> >> Jeff > I think I miscommunicated the issue previously because my understanding > of libcalls wasn't very solid. The guard is against the save/restore libcalls > specifically; other than that shrink wrapping and libcalls are fine.I think it > makes sense to leave this check because the prologue/epilogue does > something similar when using libcall save/restore: > frame->mask = 0; /* Temporarily fib that we need not save GPRs. */ Looking more closely, yea, it might have been a miscommunication between us WRT libcalls.  You're testing riscv_use_save_libcall, which is only going to kick in when we're using that special function to do register saves/restores.  While we could, in theory, shrink-wrap that as well, I don't think it's worth the additional headache. > > Since shrink wrap components are marked by testing frame->mask then > no registers should be wrapped with the libcall save/restore if I understand > correctly. That's my understanding as well after looking at the code more closely.  Essentially the mask is set to zero for the duration of the call to riscv_for_each_saved_regs which will effectively avoid shrink wrapping in that case. > > Nonetheless, I tested what happens if this guard condition is removed > and the result is that a RISCV test fails (riscv/pr95252.c). In that case > a unnecessary save/restore of a register is emitted together with > inconsistent cfi notes that make dwarf2cfi abort. ACK.  This is the kind of thing I was referring to above when I said we probably could shrink wrap the call to save/restore registers, but it's probably not worth the headache right now. I could see someone in the embedded space one day trying to tackle this problem, but I don't think we need to for this patch to go forward. > > To conclude, I believe that this makes the code in the commit fine since > it only guards against the libcall save/restore case. But I may be still > missing something about this. I think you've got it figured out reasonably well. So the final conclusion is libcalls are resolved to my satisfaction. Jeff