From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67532 invoked by alias); 23 May 2017 22:04:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 67499 invoked by uid 89); 23 May 2017 22:03:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=danger, undergoing, fund, poboxcom X-HELO: sasl.smtp.pobox.com Received: from pb-smtp1.pobox.com (HELO sasl.smtp.pobox.com) (64.147.108.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 23 May 2017 22:03:58 +0000 Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id BA1919147E; Tue, 23 May 2017 18:03:59 -0400 (EDT) Received: from pb-smtp1.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id B118F9147D; Tue, 23 May 2017 18:03:59 -0400 (EDT) Received: from [192.168.1.4] (unknown [76.215.41.237]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by pb-smtp1.pobox.com (Postfix) with ESMTPSA id B62789147C; Tue, 23 May 2017 18:03:58 -0400 (EDT) Subject: Re: [PATCH] [i386] Recompute the frame layout less often To: Bernd Edlinger References: <9aa7427e-6cfc-b603-2ec4-1f0e02ae294d@pobox.com> <20c96fa0-328b-af1a-c558-dab6652c482e@pobox.com> <8d8b4700-98eb-aaa8-7718-d603cae106e8@pobox.com> <64f15ee1-da2d-0604-2fb4-8e5fdc435c34@pobox.com> Cc: gcc-patches , Uros Bizjak From: Daniel Santos Message-ID: Date: Tue, 23 May 2017 23:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Pobox-Relay-ID: B8EACE02-4003-11E7-BF8D-EFB41968708C-06139138!pb-smtp1.pobox.com X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg01809.txt.bz2 On 05/22/2017 01:32 PM, Bernd Edlinger wrote: > On 05/19/17 05:17, Daniel Santos wrote: > >> No, I'm not at all comfortable with you making so many seemingly >> unnecessary changes to this code. (Although I wish I got this much >> feedback during my RFCs! :) I can accept the changes to >> is/count_stub_managed_reg (with some caveats), but I do not at all see >> the rationale for changing m_stub_names to a static and adding another >> dimension for the object instance -- from an OO standpoint, that's just >> bad design. Can you please share your rationale for that? >> > Hmm, sorry about that ... > I just thought it would be nice to avoid the const-cast here. Well remember const-correctness isn't about an object's internal (bitwise) state, but it's externally visible (logical) state. So a const member function need not avoid altering it's internal state if the externally visible state remains unchanged, such as when caching some result or lazy initing. I have tended to prefer using const_cast for this, isolating its use to a single const accessor function (or if () block) to leave less room for the data members to be accidentally altered in another const member function. But mutable is generally preferred over const_cast, which opens up the danger of accidentally modifying an object's logical state (especially by a subsequent edit), so using mutable is probably a better practice anyway. However, ... > This moved the m_stub_names from all 4 instances to one static > array s_stub_names. But looking at it again, I think the extra > dimension is not even necessary, because all instances share the > same data, so removing that extra dimension again will be fine. You are correct! And I see that you're new patch has already changed get_stub_name to a static member function, so great! >> Incidentally, half of the space in that array is wasted and can be >> trimmed since a given instance of xlogue_layout either supports hard >> frame pointers or doesn't, I just never got around to splitting that >> apart. (The first three enum xlogue_stub values are for without HFP and >> the last three for with.) Also, if we wanted to further reduce the >> memory footprint of xlogue_layout objects, the offset field of struct >> reginfo could be changed to int, and if we really wanted to go nuts then >> 16 bits would work for both of its fields. >> >> So for is/count_stub_managed_reg(s), you are obviously much more >> familiar with gcc, its passes and the i386 backend, but my knowledge >> level makes me very uncomfortable having the result of >> xlogue_layout::is_stub_managed_reg() determined in a way that has the >> (apparent) potential to differ from from the state at the time the last >> call to ix86_compute_frame_layout() was made; I just don't understand > I fund it technically difficult to add a HARD_REG_SET to > struct machine_function, and tried to avoid the extra overhead of > calling ix86_save_reg so often, which you also felt uncomfortable with. > > So, if you look at compute_stub_managed_regs I first saw that the > first loop can never terminate thru the "return 0", because the > registers in x86_64_ms_sysv_extra_clobbered_registers are guaranteed > to be clobbered. Then I saw that the bits in stub_managed_regs > are always added in the same sequence, thus the result depends only > on the number call_ms2sysv_extra_regs and hfp so everything is already > available in struct machine_function. > > > Thanks > Bernd. Yes, I agree with how you have refactored compute_stub_managed_regs given your rationale of not adding another header dependency to i386.h. It is only the overall scheme of calculating this outside of ix86_compute_frame_layout that I cannot validate due to my not having a good understanding of what can and cannot change in between the time that ix86_compute_frame_layout is last called and the last call to is_stub_managed_regs(). As Uros said, my patch set touches a "delicate part of the compiler, where lots of code-paths cross each other (and we have had quite some hard-to-fix bugs in this area)" (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01924.html). I wrote it the way I did with my understanding of what was safe to do and your alterations move it's functionality outside of that understanding. So if you say that this won't break it, then I will have to trust you (and the testsuite) on that. On that note, the tests are undergoing some change and bug fixes and I'm planning on adding more tests to validate non-breakage with various other stack frame-related options and probably additional tests (and test options) triggered by GCC_TEST_RUN_EXPENSIVE or some such. Daniel