From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x533.google.com (mail-pg1-x533.google.com [IPv6:2607:f8b0:4864:20::533]) by sourceware.org (Postfix) with ESMTPS id C0D633858D32 for ; Tue, 18 Oct 2022 15:57:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C0D633858D32 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-pg1-x533.google.com with SMTP id u71so13701248pgd.2 for ; Tue, 18 Oct 2022 08:57:39 -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=Og90Y0k/AEdR+Mc4pklvg/AyC8GP/QXx0SEu0gz/Em4=; b=GoJ7mMeumG33hE3VPxrR0fJhVD+5AGPX9RjKy6PQpF8OwSLy6oihCq5pBoE+D8XQKV J4ZD0Y8hkscW7jOBjXHK70Z9eXjk8fMAPH0jLwK+OGLUf1MJhsjgeJDq7kzrVkifB8+Z OHObA/VCKKL6YLZ++1sSokrKhvsTjYUODk7Bxc95iNwM8svGy2stAqxmUDZeANb5JNCQ o3qYXNnIr4lqqbrkrbra4V5TQkJX+5So5fIH6I7emvuc4800NsBWrQJC90rRsiHSZ3lu 7ng7EITdDckCAdbpz7GiB5a+HqV0QOhnepF6ORGW60LYTdHb0VGea5tzI+ZZSErFUIZh GaFw== 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=Og90Y0k/AEdR+Mc4pklvg/AyC8GP/QXx0SEu0gz/Em4=; b=zJ0aFfkCELn8tqI/J3b4Nb+zNyvlJqF0qGuiaDnO9TGzo6k7kwCxerxHMghiP7QPA7 7W9rAjTCGanpdZTVC5hlHARuvW9+jpyb6/ktzquICdEHOQ52j4txa23o9YoPd7i5IzB3 +DjxNaOSmV+QH6mTUmqH7gwUkLnd9CnY69Uqlhzd4xQDl6JseixJwt7cC856qIJp9Ofe XZp0OH6+1xZ8eX33BAiBqEI9F9yqzLH+soRHny4B8FWX7JjTVPTEQ2d3kusScpbhrG5k mbsaqeYI5Ymqa9Xsr91go+QBuXTUadCErrrZW/B+4CoKHjjwCSKwHmji2xMy2jSruJ6r Hwuw== X-Gm-Message-State: ACrzQf25pfIaXxjRpO/gXA9NdWrkw6CZaBvs4T5uFCU5bMtrE/zOvYK1 Oi6MiOA0EtF4uSpRLDs69TyJcw== X-Google-Smtp-Source: AMsMyM5Srm8hUyp4X0RIUjxwD1pFxC7A+KTj3f6+5dQO7ivtOFCTvDmlOvTdCQmPwmAS+9ApbVUQig== X-Received: by 2002:a05:6a00:1596:b0:563:9a1a:b5b0 with SMTP id u22-20020a056a00159600b005639a1ab5b0mr3598861pfk.38.1666108658704; Tue, 18 Oct 2022 08:57:38 -0700 (PDT) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id e15-20020a170902784f00b00183ba0fd54dsm8837501pln.262.2022.10.18.08.57.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Oct 2022 08:57:38 -0700 (PDT) Message-ID: <2e2ca1dc-902e-23ca-babf-11a4978530ef@ventanamicro.com> Date: Tue, 18 Oct 2022 09:57:37 -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 , Palmer Dabbelt Cc: Vineet Gupta , gcc-patches@gcc.gnu.org, Kito Cheng , philipp.tomsich@vrull.eu References: <20220906103902.2719585-1-manolis.tsamis@vrull.eu> From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.3 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: Just a couple more comments in-line. On 10/18/22 09:18, Manolis Tsamis wrote: > >>> +/* Implement TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS. */ >>> + >>> +static sbitmap >>> +riscv_get_separate_components (void) >>> +{ >>> + HOST_WIDE_INT offset; >>> + sbitmap components = sbitmap_alloc (FIRST_PSEUDO_REGISTER); >>> + bitmap_clear (components); >>> + >>> + if (riscv_use_save_libcall (&cfun->machine->frame) >>> + || cfun->machine->interrupt_handler_p) >> riscv_use_save_libcall() already checks interrupt_handler_p, so that's >> redundant. That said, I'm not sure riscv_use_save_libcall() is the >> right check here as unless I'm missing something we don't have all those >> other constraints when shrink-wrapping. >> > riscv_use_save_libcall returns false when interrupt_handler_p is true, so the > check for interrupt_handler_p in the branch is not redundant in this case. > > I encountered some issues when shrink wrapping and libcall was used in the same > function. Thinking that libcall replaces the prologue/epilogue I didn't see a > reason to have both at the same time and hence I opted to disable > shrink wrapping in that case. From my understanding this should be harmless? I would have expected things to work fine with libcalls, perhaps with the exception of the save/restore libcalls.  So that needs deeper investigation. >> >> It seems kind of clunky to have two copies of all these loops (and we'll >> need a third to make this work with the V stuff), but we've got that >> issue elsewhere in the port so I don't think you need to fix it here >> (though the V stuff will be there for the v2, so you'll need the third >> copy of each loop). >> > Indeed, I was following the other ports here. Do you think it would be > better to refactor this when the code for the V extension is added? > By taking into account what code will be needed for V, a proper refactored > function could be made to handle all cases. I think refactoring when V gets added would be fine.  While we could probably refactor it correctly now (it isn't terribly complex code after all), but we're more likely to get it right with the least amount of work if we do it when V is submitted. >> Either way, this deserves a test case. I think it should be possible to >> write one by introducing some register pressure around a >> shrink-wrappable block that needs a long stack offset and making sure >> in-flight registers don't get trashed. >> > I tried to think of some way to introduce a test like that but couldn't and > I don't see how it would be done. Shrink wrapping only affects saved registers > so there are always available temporaries that are not affected by > shrink wrapping. > (Register pressure should be irrelevant in this case if I understand correctly). > Also the implementation checks for SMALL_OPERAND (offset) shrink wrapping > should be unaffected from long stack offsets. If you see some way to write > a test for that based on what I explained please explain how I could do that. I think the register pressure was just to ensure that some saves were needed to trigger an attempt to shrink wrap something.  You'd also need something to eat stack space (local array which gets referenced as an asm operand, but where the asm doesn't generate any code perhaps)?  Whether or not that works depends on stack layout though which I don't know well enough for riscv. Generally looks pretty good though. Jeff