From patchwork Wed Apr 6 07:47:09 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 65152 Delivered-To: patch@linaro.org Received: by 10.112.199.169 with SMTP id jl9csp894211lbc; Wed, 6 Apr 2016 00:47:42 -0700 (PDT) X-Received: by 10.98.19.151 with SMTP id 23mr36344506pft.31.1459928862730; Wed, 06 Apr 2016 00:47:42 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id zo2si2814475pac.221.2016.04.06.00.47.42 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 06 Apr 2016 00:47:42 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-424429-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-424429-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-424429-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=U3f4nO2uDFd0uHZaec ahduUIkKlOHhWk06eguTozkj7P94qCCkOt/cmSK35hMQU8FN9Ok/TU2sLQAIs/hM OltE2a9jjka9GNYegVF8NvcKDEYeqiUjoHaDWPOtnh1J6dCQ3pGLRs+k5RtkSnrS OJTOMEUfYLmwFLeehahZ4zrx4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=vHKWsfDdslJwru41wESwwcbs cG0=; b=lhkwOG3+Qm8MbCCN2I/cumAOef85y4Vwy0Q4RtKqPsUg6tNKVoV/ol1R o4Dof6sVAlm67wN5DG1I1BsOL8U72IoZiOSvn24OAM0GyJRFDJcGlLhTZNj4z5MJ 0FftgEWNZNP0c9fBPfW9I+P3WzBD7+wa2fR5Hh1Ez59OwOws21o= Received: (qmail 36318 invoked by alias); 6 Apr 2016 07:47:28 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 36288 invoked by uid 89); 6 Apr 2016 07:47:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=million, expenses, ten, late X-HELO: mail-io0-f170.google.com Received: from mail-io0-f170.google.com (HELO mail-io0-f170.google.com) (209.85.223.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 06 Apr 2016 07:47:12 +0000 Received: by mail-io0-f170.google.com with SMTP id 2so46942952ioy.1 for ; Wed, 06 Apr 2016 00:47:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=9pD9I86CIY3VhdwHd8wNzpaXC72PpKorr65ngkgXqQk=; b=O8morFFdB+WgGprF04UdKEhaG0JJFKz0MQJQxhdXVGr518Ia0LuBBD8IfOBhhucqYA nx+xUF0iibcDXEPavGmp2bcV15IMLKnDD2REzT7C77KhoZVyrBb5khn6l6AaQcXrXbQU K5OxBV8Xt9fI0AWdr53WC/2LAJ9kdPRNylsu74e5oJ2lRGGGo6abKfsAjoKOHWXYS9gR acfkJFvZ27FtSh9bi7SbosKNuF0INg5vbAebUn7FVqqPnlKX7GcLADvkze4AimjCj+1y GGjUNiempFdfHfobhldQT2gCifVWDILlCfQRgVe0QwlYSEoIHMxV4wZXsYyOQ+BF+1e+ HaQw== X-Gm-Message-State: AD7BkJISTO9CkQlvRFC5dL12J+cqA54r6iJa2avUjkfpvRCQc+2KVRnGnNaKz+zG+LnYKankw2uoMM4xGT9YHYAD MIME-Version: 1.0 X-Received: by 10.107.130.148 with SMTP id m20mr3309502ioi.137.1459928829933; Wed, 06 Apr 2016 00:47:09 -0700 (PDT) Received: by 10.36.196.5 with HTTP; Wed, 6 Apr 2016 00:47:09 -0700 (PDT) In-Reply-To: References: <20160404120030.GD14122@kam.mff.cuni.cz> <20160404141436.GB95176@kam.mff.cuni.cz> Date: Wed, 6 Apr 2016 13:17:09 +0530 Message-ID: Subject: Re: [RFC] introduce --param max-lto-partition for having an upper bound on partition size From: Prathamesh Kulkarni To: Richard Biener Cc: Jan Hubicka , gcc Patches , Ramana Radhakrishnan X-IsSubscribed: yes 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 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) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 9e54bb7..f0de7ec 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -9477,6 +9477,11 @@ Size of minimal partition for WHOPR (in estimated instructions). This prevents expenses of splitting very small programs into too many partitions. +@item lto-max-partition +Size of max partition for WHOPR (in estimated instructions). +to provide an upper bound for individual size of partition. +Meant to be used only with balanced partitioning. + @item cxx-max-namespaces-for-diagnostic-help The maximum number of namespaces to consult for suggestions when C++ name lookup fails for an identifier. The default is 1000. diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c index 9eb63c2..d385dd9 100644 --- a/gcc/lto/lto-partition.c +++ b/gcc/lto/lto-partition.c @@ -447,7 +447,7 @@ add_sorted_nodes (vec &next_nodes, ltrans_partition partition) and in-partition calls was reached. */ void -lto_balanced_map (int n_lto_partitions) +lto_balanced_map (int n_lto_partitions, bool honor_max_partition) { int n_nodes = 0; int n_varpool_nodes = 0, varpool_pos = 0, best_varpool_pos = 0; @@ -511,6 +511,9 @@ 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); @@ -719,7 +723,9 @@ lto_balanced_map (int n_lto_partitions) best_cost, best_internal, best_i); /* Partition is too large, unwind into step when best cost was reached and start new partition. */ - if (partition->insns > 2 * partition_size) + if (partition->insns > 2 * partition_size + || (honor_max_partition + && partition->insns > PARAM_VALUE (MAX_PARTITION_SIZE))) { if (best_i != i) { diff --git a/gcc/lto/lto-partition.h b/gcc/lto/lto-partition.h index 31e3764..a55594c 100644 --- a/gcc/lto/lto-partition.h +++ b/gcc/lto/lto-partition.h @@ -35,7 +35,7 @@ extern vec ltrans_partitions; void lto_1_to_1_map (void); void lto_max_map (void); -void lto_balanced_map (int); +void lto_balanced_map (int, bool honor_max_partition); void lto_promote_cross_file_statics (void); void free_ltrans_partitions (void); void lto_promote_statics_nonwpa (void); diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c index 9dd513f..ef2ce15 100644 --- a/gcc/lto/lto.c +++ b/gcc/lto/lto.c @@ -3117,9 +3118,9 @@ do_whole_program_analysis (void) else if (flag_lto_partition == LTO_PARTITION_MAX) lto_max_map (); else if (flag_lto_partition == LTO_PARTITION_ONE) - lto_balanced_map (1); + lto_balanced_map (1, false); else if (flag_lto_partition == LTO_PARTITION_BALANCED) - lto_balanced_map (PARAM_VALUE (PARAM_LTO_PARTITIONS)); + lto_balanced_map (PARAM_VALUE (PARAM_LTO_PARTITIONS), true); else gcc_unreachable (); diff --git a/gcc/params.def b/gcc/params.def index 9362c15..b5da384 100644 --- a/gcc/params.def +++ b/gcc/params.def @@ -1029,6 +1029,11 @@ DEFPARAM (MIN_PARTITION_SIZE, "Minimal size of a partition for LTO (in estimated instructions).", 1000, 0, 0) +DEFPARAM (MAX_PARTITION_SIZE, + "lto-max-partition", + "Maximal size of a partition for LTO (in estimated instructions).", + 1000000, 0, INT_MAX) + /* Diagnostic parameters. */ DEFPARAM (CXX_MAX_NAMESPACES_FOR_DIAGNOSTIC_HELP,