From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65893 invoked by alias); 6 Apr 2016 08:14:47 -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 65877 invoked by uid 89); 6 Apr 2016 08:14:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=million, ten X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Wed, 06 Apr 2016 08:14:36 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 6A544AB9D; Wed, 6 Apr 2016 08:14:32 +0000 (UTC) Date: Wed, 06 Apr 2016 08:14:00 -0000 From: Richard Biener To: Prathamesh Kulkarni cc: Jan Hubicka , gcc Patches , Ramana Radhakrishnan Subject: Re: [RFC] introduce --param max-lto-partition for having an upper bound on partition size In-Reply-To: Message-ID: References: <20160404120030.GD14122@kam.mff.cuni.cz> <20160404141436.GB95176@kam.mff.cuni.cz> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2016-04/txt/msg00271.txt.bz2 On Wed, 6 Apr 2016, Prathamesh Kulkarni wrote: > On 5 April 2016 at 18:28, Richard Biener wrote: > > On Tue, 5 Apr 2016, Prathamesh Kulkarni wrote: > > > >> On 5 April 2016 at 16:58, Richard Biener wrote: > >> > On Tue, 5 Apr 2016, Prathamesh Kulkarni wrote: > >> > > >> >> On 4 April 2016 at 19:44, Jan Hubicka wrote: > >> >> > > >> >> >> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c > >> >> >> index 9eb63c2..bc0c612 100644 > >> >> >> --- a/gcc/lto/lto-partition.c > >> >> >> +++ b/gcc/lto/lto-partition.c > >> >> >> @@ -511,9 +511,20 @@ lto_balanced_map (int n_lto_partitions) > >> >> >> varpool_order.qsort (varpool_node_cmp); > >> >> >> > >> >> >> /* Compute partition size and create the first partition. */ > >> >> >> + if (PARAM_VALUE (MIN_PARTITION_SIZE) > PARAM_VALUE (MAX_PARTITION_SIZE)) > >> >> >> + fatal_error (input_location, "min partition size cannot be greater than max partition size"); > >> >> >> + > >> >> >> partition_size = total_size / n_lto_partitions; > >> >> >> if (partition_size < PARAM_VALUE (MIN_PARTITION_SIZE)) > >> >> >> partition_size = PARAM_VALUE (MIN_PARTITION_SIZE); > >> >> >> + else if (partition_size > PARAM_VALUE (MAX_PARTITION_SIZE)) > >> >> >> + { > >> >> >> + n_lto_partitions = total_size / PARAM_VALUE (MAX_PARTITION_SIZE); > >> >> >> + if (total_size % PARAM_VALUE (MAX_PARTITION_SIZE)) > >> >> >> + n_lto_partitions++; > >> >> >> + partition_size = total_size / n_lto_partitions; > >> >> >> + } > >> >> > > >> >> > lto_balanced_map actually works in a way that looks for cheapest cutpoint in range > >> >> > 3/4*parittion_size to 2*partition_size and picks the cheapest range. > >> >> > Setting partition_size to this value will thus not cause partitioner to produce smaller > >> >> > partitions only. I suppose modify the conditional: > >> >> > > >> >> > /* Partition is too large, unwind into step when best cost was reached and > >> >> > start new partition. */ > >> >> > if (partition->insns > 2 * partition_size) > >> >> > > >> >> > and/or in the code above set the partition_size to half of total_size/max_size. > >> >> > > >> >> > I know this is somewhat sloppy. This was really just first cut implementation > >> >> > many years ago. I expected to reimplement it marter soon, but then there was > >> >> > never really a need for it (I am trying to avoid late IPA optimizations so the > >> >> > partitioning decisions should mostly affect compile time performance only). > >> >> > If ARM is more sensitive for partitining, perhaps it would make sense to try to > >> >> > look for something smarter. > >> >> > > >> >> >> + > >> >> >> npartitions = 1; > >> >> >> partition = new_partition (""); > >> >> >> if (symtab->dump_file) > >> >> >> diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c > >> >> >> index 9dd513f..294b8a4 100644 > >> >> >> --- a/gcc/lto/lto.c > >> >> >> +++ b/gcc/lto/lto.c > >> >> >> @@ -3112,6 +3112,12 @@ do_whole_program_analysis (void) > >> >> >> timevar_pop (TV_WHOPR_WPA); > >> >> >> > >> >> >> timevar_push (TV_WHOPR_PARTITIONING); > >> >> >> + > >> >> >> + if (flag_lto_partition != LTO_PARTITION_BALANCED > >> >> >> + && PARAM_VALUE (MAX_PARTITION_SIZE) != INT_MAX) > >> >> >> + fatal_error (input_location, "--param max-lto-partition should only" > >> >> >> + " be used with balanced partitioning\n"); > >> >> >> + > >> >> > > >> >> > I think we should wire in resonable MAX_PARTITION_SIZE default. THe value you > >> >> > found experimentally may be a good start. For that reason we can't really > >> >> > refuse a value when !LTO_PARTITION_BALANCED. Just document it as parameter for > >> >> > balanced partitioning only and add a parameter to lto_balanced_map specifying whether > >> >> > this param should be honored (because the same path is used for partitioning to one partition) > >> >> > > >> >> > Otherwise the patch looks good to me modulo missing documentation. > >> >> Thanks for the review. I have updated the patch. > >> >> Does this version look OK ? > >> >> I had randomly chosen 10000, not sure if that's an appropriate value > >> >> for default. > >> > > >> > I think it's way too small. This is roughly the number of GIMPLE stmts > >> > (thus roughly the number of instructions). So with say a 8 byte > >> > instruction format it is on the order of 80kB. You'd want to have a > >> > default of at least several ten times of large-unit-insns (also 10000). > >> > I'd choose sth like 1000000 (one million). I find the lto-min-partition > >> > number quite small as well (and up it by a factor of 10). > >> Done in this version. > > > > I'd do that separately. > > > > Please no default parameter for lto_balanced_map (), instead change > > all callers. > > > >> Is it OK after bootstrap+test ? > > > > Note that this is for stage1 only. I'll leave approval to Honza > > (also verification of the default max param - not sure if for example > > chromium or firefox should/will be split to more than 32 partitions > > with the patch) > Removed default parameter in this version. I verified with the patch > for chromium LTO build: > n_lto_partitions == 32, ltrans_partitions.length() == 31 Just noticed that lto_balanced_map already gets PARAM_LTO_PARTITIONS, so why not pass it PARAM_MAX_PARTITION_SIZE or 0 (as magic value for unlimited) instead of a bool parameter? Richard. > Thanks, > Prathamesh > > > > Richard. > > > >> Thanks, > >> Prathamesh > >> > > >> > Richard. > >> > > >> >> I have a silly question about partitioning: Does it hamper > >> >> transformations on ipa optimizations if caller and > >> >> callee get placed in separate partitions ? For instance if callee is > >> >> supposed to be inlined > >> >> into caller, would inlining still take place if callee and caller get > >> >> placed in separate partitions ? > >> >> I tried with a trivial example with -flto-partition=max > >> >> which created 3 partitions for 3 functions (bar, foo and main), and it was > >> >> able to inline bar into foo and foo into main. I am not sure how that happens. > >> >> I thought ltrans can perform transformations on functions only within > >> >> a single partition > >> >> and not across partitions ? > >> >> > >> >> Thanks, > >> >> Prathamesh > >> >> > > >> >> > Honza > >> >> > >> > > >> > -- > >> > Richard Biener > >> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > >> > > > > -- > > Richard Biener > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)