[PATCH] Fix noreorder symbol partitioning reversion.

[PATCH] Fix noreorder symbol partitioning reversion.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

Hi.

The patch is fixes a regression in libgcrypt package where
we incorrectly forget to stream out a definition of a no-reorder symbol.
It's caused by LTO balanced map reversion, where we do not revert
also best_noreorder_pos.

It's pre-approved patch by Honza and I'm going to install it.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Thanks,
Martin


gcc/lto/ChangeLog:

2020-01-16  Martin Liska  <mliska@suse.cz>

	* lto-partition.c (lto_balanced_map): Remember
	best_noreorder_pos and then restore to it
	when we revert.
---
  gcc/lto/lto-partition.c | 3 +++
  1 file changed, 3 insertions(+)



[-- Attachment #2: 0001-Fix-noreorder-symbol-partitioning-reversion.patch --]
[-- Type: text/x-patch, Size: 1128 bytes --]

diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
index 3a9990903c7..8e0488ab13e 100644
--- a/gcc/lto/lto-partition.c
+++ b/gcc/lto/lto-partition.c
@@ -471,6 +471,7 @@ void
 lto_balanced_map (int n_lto_partitions, int max_partition_size)
 {
   int n_varpool_nodes = 0, varpool_pos = 0, best_varpool_pos = 0;
+  int best_noreorder_pos = 0;
   auto_vec <cgraph_node *> order (symtab->cgraph_count);
   auto_vec<cgraph_node *> noreorder;
   auto_vec<varpool_node *> varpool_order;
@@ -732,6 +733,7 @@ lto_balanced_map (int n_lto_partitions, int max_partition_size)
 	  best_i = i;
 	  best_n_nodes = lto_symtab_encoder_size (partition->encoder);
 	  best_varpool_pos = varpool_pos;
+	  best_noreorder_pos = noreorder_pos;
 	}
       if (dump_file)
 	fprintf (dump_file, "Step %i: added %s, size %i, "
@@ -752,6 +754,7 @@ lto_balanced_map (int n_lto_partitions, int max_partition_size)
 			 i - best_i, best_i);
 	      undo_partition (partition, best_n_nodes);
 	      varpool_pos = best_varpool_pos;
+	      noreorder_pos = best_noreorder_pos;
 	    }
 	  gcc_assert (best_size == partition->insns);
 	  i = best_i;

Re: [PATCH] Fix noreorder symbol partitioning reversion.

[Jan Hubicka]

> Hi.
> 
> The patch is fixes a regression in libgcrypt package where
> we incorrectly forget to stream out a definition of a no-reorder symbol.
> It's caused by LTO balanced map reversion, where we do not revert
> also best_noreorder_pos.
> 
> It's pre-approved patch by Honza and I'm going to install it.
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Thanks,
> Martin
> 
> 
> gcc/lto/ChangeLog:
> 
> 2020-01-16  Martin Liska  <mliska@suse.cz>
> 
> 	* lto-partition.c (lto_balanced_map): Remember
> 	best_noreorder_pos and then restore to it
> 	when we revert.

Thanks,
also please backport it to release branches once it survives some
testing in mainline.

Honza

Re: [PATCH] Fix noreorder symbol partitioning reversion.

[Jeff Law]

On Thu, 2020-01-16 at 20:43 +0100, Martin Liška wrote:
> Hi.
> 
> The patch is fixes a regression in libgcrypt package where
> we incorrectly forget to stream out a definition of a no-reorder symbol.
> It's caused by LTO balanced map reversion, where we do not revert
> also best_noreorder_pos.
> 
> It's pre-approved patch by Honza and I'm going to install it.
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Thanks,
> Martin
> 
> 
> gcc/lto/ChangeLog:
> 
> 2020-01-16  Martin Liska  <mliska@suse.cz>
> 
> 	* lto-partition.c (lto_balanced_map): Remember
> 	best_noreorder_pos and then restore to it
> 	when we revert.
Thank you!  That addressed several niggling failures.

jeff
> 

Re: [PATCH] Fix noreorder symbol partitioning reversion.

[Jan Hubicka]

> On Thu, 2020-01-16 at 20:43 +0100, Martin Liška wrote:
> > Hi.
> > 
> > The patch is fixes a regression in libgcrypt package where
> > we incorrectly forget to stream out a definition of a no-reorder symbol.
> > It's caused by LTO balanced map reversion, where we do not revert
> > also best_noreorder_pos.
> > 
> > It's pre-approved patch by Honza and I'm going to install it.
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > 
> > Thanks,
> > Martin
> > 
> > 
> > gcc/lto/ChangeLog:
> > 
> > 2020-01-16  Martin Liska  <mliska@suse.cz>
> > 
> > 	* lto-partition.c (lto_balanced_map): Remember
> > 	best_noreorder_pos and then restore to it
> > 	when we revert.
> Thank you!  That addressed several niggling failures.

Yep, it is quite intereesting the bug survived multiple releases since
the initial Andi's kernel work.
On the other hand it reproduces only if you mix in -O0 modules or use
explicit -fno-toplevel-reorder.  So if you see it in packages it is
worth to investigate if build flags are not broken.
-O0 -flto is not the most powerful optimization to do.

Honza
> 
> jeff
> > 
> 

Re: [PATCH] Fix noreorder symbol partitioning reversion.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 98 bytes --]

Hi.

I've just tested the backport on both GCC 8 and 9 branches.

I'm going to install it.
Martin

[-- Attachment #2: 0001-Backport-f48c6014133c8989702458f9082e34ba6dd326d4.patch --]
[-- Type: text/x-patch, Size: 1614 bytes --]

From 4a856551c0f969be6525371892277c206b4181da Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 20 Jan 2020 11:16:21 +0100
Subject: [PATCH] Backport f48c6014133c8989702458f9082e34ba6dd326d4

gcc/lto/ChangeLog:

2020-01-16  Martin Liska  <mliska@suse.cz>

	* lto-partition.c (lto_balanced_map): Remember
	best_noreorder_pos and then restore to it
	when we revert.
---
 gcc/lto/lto-partition.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
index bf88d8a4e65..ed6f30cecb2 100644
--- a/gcc/lto/lto-partition.c
+++ b/gcc/lto/lto-partition.c
@@ -507,6 +507,7 @@ void
 lto_balanced_map (int n_lto_partitions, int max_partition_size)
 {
   int n_varpool_nodes = 0, varpool_pos = 0, best_varpool_pos = 0;
+  int best_noreorder_pos = 0;
   auto_vec <cgraph_node *> order (symtab->cgraph_count);
   auto_vec<cgraph_node *> noreorder;
   auto_vec<varpool_node *> varpool_order;
@@ -769,6 +770,7 @@ lto_balanced_map (int n_lto_partitions, int max_partition_size)
 	  best_i = i;
 	  best_n_nodes = lto_symtab_encoder_size (partition->encoder);
 	  best_varpool_pos = varpool_pos;
+	  best_noreorder_pos = noreorder_pos;
 	}
       if (symtab->dump_file)
 	fprintf (symtab->dump_file, "Step %i: added %s/%i, size %i, "
@@ -789,6 +791,7 @@ lto_balanced_map (int n_lto_partitions, int max_partition_size)
 			 i - best_i, best_i);
 	      undo_partition (partition, best_n_nodes);
 	      varpool_pos = best_varpool_pos;
+	      noreorder_pos = best_noreorder_pos;
 	    }
 	  gcc_assert (best_size == partition->insns);
 	  i = best_i;
-- 
2.24.1