From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125450 invoked by alias); 30 Oct 2015 14:08:21 -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 125430 invoked by uid 89); 30 Oct 2015 14:08:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 30 Oct 2015 14:08:19 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 3564C8E399; Fri, 30 Oct 2015 14:08:18 +0000 (UTC) Received: from localhost.localdomain (vpn1-7-116.ams2.redhat.com [10.36.7.116]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9UE8GkG020548; Fri, 30 Oct 2015 10:08:16 -0400 Subject: Re: using scratchpads to enhance RTL-level if-conversion: revised patch To: Abe , "gcc-patches@gcc.gnu.org" , Sebastian Pop , Kyrill Tkachov , Jakub Jelinek References: <024301d11106$2379b5f0$6a6d21d0$@samsung.com> <562FFC2D.1020602@yahoo.com> From: Bernd Schmidt Message-ID: <563379CF.3080209@redhat.com> Date: Fri, 30 Oct 2015 14:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <562FFC2D.1020602@yahoo.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg03403.txt.bz2 (Jakub Cc'd because of code he added for PR23567). On 10/27/2015 11:35 PM, Abe wrote: > Thanks for all your feedback. I have integrated as much of it as I > could in the available time. Unfortunately not all of it - I still think we need to have a better strategy of selecting a scratchpad than a newly allocated stack slot. There are sufficiently many options. > * ifcvt.c (noce_mem_write_may_go_wrong_even_with_scratchpads): > New. ChangeLog doesn't correspond to the patch. If the function actually existed I'd reject it for a way overlong identifier. > * target.h: New enum named "RTL_ifcvt_when_to_use_scratchpads". Please follow ChangeLog writing guidelines. > -/* Return true if a write into MEM may trap or fault. */ > +/* Return true if a write into MEM may trap or fault > + even in the presence of scratchpad support. */ I still think this comment is fairly useless and needs to better describe what it is actually doing. > static bool > -noce_mem_write_may_trap_or_fault_p (const_rtx mem) > +noce_mem_write_may_trap_or_fault_p_1 (const_rtx mem) For what this ends up doing, I think a name like "unsafe_address_p" would be better. Also, I think the code in there is really dubious - it tries to look for SYMBOL_REFs which are in decl_readonly_section, but shouldn't that be unnecessary given the test for MEM_READONLY_P? It's completely unreliable anyway since the address could be loaded into a register (on most targets it would be) and we'd never see the SYMBOL_REF in that function. > +/* Return true if a write into MEM may trap or fault > + without scratchpad support. */ Just keep the previous comment without mentioning scratchpads. > +static bool > +noce_mem_write_may_trap_or_fault_p (const_rtx mem) > +{ > + if (may_trap_or_fault_p (mem)) > + return true; > + > + return noce_mem_write_may_trap_or_fault_p_1 (mem); > +} > + if (RTL_ifcvt_use_spads_as_per_profile > + == targetm.RTL_ifcvt_when_to_use_scratchpads > + && (PROFILE_ABSENT == profile_status_for_fn (cfun) > + || PROFILE_GUESSED == profile_status_for_fn (cfun) > + || predictable_edge_p (then_edge) > + || ! maybe_hot_bb_p (cfun, then_bb))) > + return FALSE; I guess this is slightly better than no cost estimate at all. > + if (noce_mem_write_may_trap_or_fault_p_1 (orig_x) So why do you want to call this function anyway? Doesn't the scratchpad technique protect against storing to a bad address? > + const size_t MEM_size = MEM_SIZE (orig_x); No uppercase letters for variables and such. Just use "sz" or "size" for brevity. > + biggest_spad = assign_stack_local (GET_MODE (orig_x), Still the same problems - this is the least attractive choice of a scratchpad location, and the code may end up allocating more scratchpads than you need. > + emit_insn_before_setloc (seq, if_info->jump, > + INSN_LOCATION (if_info->insn_a)); Formatting? Could be mail client damage. > +DEFHOOKPOD > +(RTL_ifcvt_when_to_use_scratchpads, > +"*", > +enum RTL_ifcvt_when_to_use_scratchpads, > RTL_ifcvt_use_spads_as_per_profile) > + No caps, and maybe a less clumsy name. > +enum RTL_ifcvt_when_to_use_scratchpads { > + RTL_ifcvt_never_use_scratchpads = 0, > + RTL_ifcvt_always_use_scratchpads, > + RTL_ifcvt_use_spads_as_per_profile > +}; Likewise. Maybe enum ifcvt_scratchpads_strategy { scratchpad_never, scratchpad_always, scratchpad_unpredictable }; So, still a NACK from my side. Bernd