diff mbox

[RFC] introduce --param max-lto-partition for having an upper bound on partition size

Message ID CAAgBjM=3k7YMB4AvDeFAgrBJpLKeJiQPGFHtyNT9pXqLqo2LGQ@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni April 4, 2016, 1:27 p.m. UTC
On 4 April 2016 at 17:30, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Um not sure if I understood correctly.
>> > Do we want to constrain individual partition size by adding parameter
>> > lto-max-partition
>> > for balanced partitioning but not for -flto-partition=one
>> > case (since latter would also change semantics of =one) ?
>>
>> Yes, I think so.
>
> Yep, I agree.  Having partition one that produces multiple partitions doesn't seem to make much sense.
> Given that we have such not so predictable target specific limits on size of single translation unit
> we can handle, I suppose adding a resonable limit to the default balanced partitioning makes more sense.
> One can always push the behaviour you intend by setting max partitions to 1 (I suppose max size should
> have precedence over max partitions)
Thanks for the suggestions, I have updated the patch.
Is it OK if it passes bootstrap+test ?

Thanks,
Prathamesh
>
> Honza
>>
>> Richard.
>>
>> > Thanks,
>> > Prathamesh
>> > >
>> > > Richard.
>> > >
>> > > --
>> > > 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)

Comments

Jan Hubicka April 4, 2016, 2:14 p.m. UTC | #1
> 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.

Honza
diff mbox

Patch

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;
+    }
+
   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");
+
   if (flag_lto_partition == LTO_PARTITION_1TO1)
     lto_1_to_1_map ();
   else if (flag_lto_partition == LTO_PARTITION_MAX)
diff --git a/gcc/params.def b/gcc/params.def
index 9362c15..b6055ff 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).",
+	  INT_MAX, 0, INT_MAX)
+
 /* Diagnostic parameters.  */
 
 DEFPARAM (CXX_MAX_NAMESPACES_FOR_DIAGNOSTIC_HELP,