From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
To: Richard Biener <rguenther@suse.de>
Cc: Jan Hubicka <hubicka@ucw.cz>,
gcc Patches <gcc-patches@gcc.gnu.org>,
Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Subject: Re: [RFC] introduce --param max-lto-partition for having an upper bound on partition size
Date: Tue, 05 Apr 2016 12:55:00 -0000 [thread overview]
Message-ID: <CAAgBjMn+JGNgkFMwhUtYCVRhKfGRCicOK0F5o6ppwgbbJMkzRA@mail.gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1604051324100.13384@t29.fhfr.qr>
[-- Attachment #1: Type: text/plain, Size: 5053 bytes --]
On 5 April 2016 at 16:58, Richard Biener <rguenther@suse.de> wrote:
> On Tue, 5 Apr 2016, Prathamesh Kulkarni wrote:
>
>> On 4 April 2016 at 19:44, Jan Hubicka <hubicka@ucw.cz> 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.
Is it OK after bootstrap+test ?
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 <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
[-- Attachment #2: patch-4.diff --]
[-- Type: text/plain, Size: 3867 bytes --]
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<symtab_node *> &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,9 +511,13 @@ 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);
+
npartitions = 1;
partition = new_partition ("");
if (symtab->dump_file)
@@ -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..2992bee 100644
--- a/gcc/lto/lto-partition.h
+++ b/gcc/lto/lto-partition.h
@@ -35,7 +35,7 @@ extern vec<ltrans_partition> 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 = true);
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..82bd9b3 100644
--- a/gcc/lto/lto.c
+++ b/gcc/lto/lto.c
@@ -3112,12 +3112,13 @@ do_whole_program_analysis (void)
timevar_pop (TV_WHOPR_WPA);
timevar_push (TV_WHOPR_PARTITIONING);
+
if (flag_lto_partition == LTO_PARTITION_1TO1)
lto_1_to_1_map ();
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));
else
diff --git a/gcc/params.def b/gcc/params.def
index 9362c15..97e41aa 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1027,7 +1027,12 @@ DEFPARAM (PARAM_LTO_PARTITIONS,
DEFPARAM (MIN_PARTITION_SIZE,
"lto-min-partition",
"Minimal size of a partition for LTO (in estimated instructions).",
- 1000, 0, 0)
+ 10000, 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. */
[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 589 bytes --]
2016-04-05 Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
* params.def (MAX_PARTITION_SIZE): New param.
(MIN_PARTITION_SIZE): Change default value to 10000.
* lto/lto-partition.h (lto_balanced_map): New parameter
honor_max_partition with default value true.
* lto/lto-partition.c (lto_balanced_map): New parameter
honor_max_partition with default value true.
Check if partition size is greater than max-lto-partition.
* lto/lto.c (do_whole_program_analysis): Pass false as 2nd arg to
lto_balanced_map in single partition case.
* invoke.texi: Document lto-max-partition.
next prev parent reply other threads:[~2016-04-05 12:55 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-01 13:48 Prathamesh Kulkarni
2016-04-01 17:33 ` Richard Biener
2016-04-04 7:47 ` Prathamesh Kulkarni
2016-04-04 8:26 ` Richard Biener
2016-04-04 11:19 ` Prathamesh Kulkarni
2016-04-04 11:42 ` Richard Biener
2016-04-04 12:00 ` Jan Hubicka
2016-04-04 13:28 ` Prathamesh Kulkarni
2016-04-04 14:14 ` Jan Hubicka
2016-04-05 11:11 ` Prathamesh Kulkarni
2016-04-05 11:28 ` Richard Biener
2016-04-05 12:55 ` Prathamesh Kulkarni [this message]
2016-04-05 12:58 ` Richard Biener
2016-04-06 7:47 ` Prathamesh Kulkarni
2016-04-06 8:14 ` Richard Biener
2016-04-06 8:53 ` Prathamesh Kulkarni
2016-04-06 9:07 ` Richard Biener
2016-04-06 9:24 ` Richard Biener
2016-04-25 11:58 ` Prathamesh Kulkarni
2016-04-26 11:01 ` Richard Biener
2016-04-26 20:45 ` Prathamesh Kulkarni
2016-04-27 7:28 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAgBjMn+JGNgkFMwhUtYCVRhKfGRCicOK0F5o6ppwgbbJMkzRA@mail.gmail.com \
--to=prathamesh.kulkarni@linaro.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=ramana.radhakrishnan@arm.com \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).