From: Richard Biener <rguenther@suse.de>
To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
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: Wed, 06 Apr 2016 08:14:00 -0000 [thread overview]
Message-ID: <alpine.LSU.2.11.1604061013290.13384@t29.fhfr.qr> (raw)
In-Reply-To: <CAAgBjMm-BJYLX9xSFCGZ=P4fY6TXNpe2KHCCPMPV8pE_1coeaQ@mail.gmail.com>
On Wed, 6 Apr 2016, Prathamesh Kulkarni wrote:
> On 5 April 2016 at 18:28, Richard Biener <rguenther@suse.de> wrote:
> > On Tue, 5 Apr 2016, Prathamesh Kulkarni wrote:
> >
> >> 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.
> >
> > 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 <rguenther@suse.de>
> >> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
> >>
> >
> > --
> > Richard Biener <rguenther@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
next prev parent reply other threads:[~2016-04-06 8:14 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
2016-04-05 12:58 ` Richard Biener
2016-04-06 7:47 ` Prathamesh Kulkarni
2016-04-06 8:14 ` Richard Biener [this message]
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=alpine.LSU.2.11.1604061013290.13384@t29.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=prathamesh.kulkarni@linaro.org \
--cc=ramana.radhakrishnan@arm.com \
/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).