[PATCH] Add function part to a same comdat group (PR ipa/80212).

[PATCH] Add function part to a same comdat group (PR ipa/80212).

[Martin Liška]

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

Hello.

Following patch is pre-approved by Honza, where IPA split should properly set
a comdat gruop if an original function lives in any.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Martin

[-- Attachment #2: 0001-Add-function-part-to-a-same-comdat-group-PR-ipa-8021.patch --]
[-- Type: text/x-patch, Size: 1803 bytes --]

From b876a766c2e5ffcf78fdc56e2ff5b87c1e99d30b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 6 Apr 2017 16:31:13 +0200
Subject: [PATCH] Add function part to a same comdat group (PR ipa/80212).

gcc/testsuite/ChangeLog:

2017-04-06  Martin Liska  <mliska@suse.cz>

	PR ipa/80212
	* g++.dg/ipa/pr80212.C: New test.

gcc/ChangeLog:

2017-04-06  Martin Liska  <mliska@suse.cz>

	PR ipa/80212
	* ipa-split.c (split_function): Add function part to a same comdat
	group.
---
 gcc/ipa-split.c                    |  3 +++
 gcc/testsuite/g++.dg/ipa/pr80212.C | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr80212.C

diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index da3c2c62344..ae1de6f1e63 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -1363,6 +1363,9 @@ split_function (basic_block return_bb, struct split_point *split_point,
   /* Let's take a time profile for splitted function.  */
   node->tp_first_run = cur_node->tp_first_run + 1;
 
+  if (cur_node->same_comdat_group)
+    node->add_to_same_comdat_group (cur_node);
+
   /* For usual cloning it is enough to clear builtin only when signature
      changes.  For partial inlining we however can not expect the part
      of builtin implementation to have same semantic as the whole.  */
diff --git a/gcc/testsuite/g++.dg/ipa/pr80212.C b/gcc/testsuite/g++.dg/ipa/pr80212.C
new file mode 100644
index 00000000000..60d3b613035
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr80212.C
@@ -0,0 +1,18 @@
+// PR ipa/80212
+// { dg-options "-O2 --param partial-inlining-entry-probability=403796683 -fno-early-inlining" }
+
+struct b
+{
+  virtual b *c () const;
+};
+struct d : virtual b
+{
+};
+struct e : d
+{
+  e *
+  c () const
+  {
+  }
+};
+main () { e a; }
-- 
2.12.2

Re: [PATCH] Add function part to a same comdat group (PR ipa/80212).

[Martin Liška]

The patch has been just reverted because it caused many issues:
PR80366

Martin

Re: [PATCH] Add function part to a same comdat group (PR ipa/80212).

[Martin Liška]

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

Hello.

This is my second attempt to fix the PR I worked on with Honza and Martin Jambor.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Apart from that
octoploid confirmed it can survive LLVM build. And I can build Firefox and boost with the
patch on x86_64-linux-gnu.

Ready to be installed?
Martin

[-- Attachment #2: 0001-Add-function-part-to-a-same-comdat-group-PR-ipa-8021.patch --]
[-- Type: text/x-patch, Size: 2882 bytes --]

From f8934791a3d345eb8c2c51beca07177c75e5f6ac Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 11 Apr 2017 14:22:50 +0200
Subject: [PATCH] Add function part to a same comdat group (PR ipa/80212).

gcc/ChangeLog:

2017-04-11  Martin Liska  <mliska@suse.cz>

	PR ipa/80212
	* cgraph.c (cgraph_node::dump): Dump calls_comdat_local.
	* ipa-cp.c (determine_versionability): Handle calls_comdat_local
	flags.
	* ipa-split.c (split_function): Create a local comdat symbol
	if caller is in a comdat group.

gcc/testsuite/ChangeLog:

2017-04-11  Martin Liska  <mliska@suse.cz>

	PR ipa/80212
	* g++.dg/ipa/pr80212.C: New test.
---
 gcc/cgraph.c                       |  2 ++
 gcc/ipa-cp.c                       |  2 ++
 gcc/ipa-split.c                    |  7 +++++++
 gcc/testsuite/g++.dg/ipa/pr80212.C | 18 ++++++++++++++++++
 4 files changed, 29 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr80212.C

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 92ae0910c60..e505b10e211 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -2123,6 +2123,8 @@ cgraph_node::dump (FILE *f)
     fprintf (f, " only_called_at_exit");
   if (tm_clone)
     fprintf (f, " tm_clone");
+  if (calls_comdat_local)
+    fprintf (f, " calls_comdat_local");
   if (icf_merged)
     fprintf (f, " icf_merged");
   if (merged_comdat)
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 0b408149a88..756a335661d 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -614,6 +614,8 @@ determine_versionability (struct cgraph_node *node,
      decloned constructors, inlining is always better anyway.  */
   else if (node->comdat_local_p ())
     reason = "comdat-local function";
+  else if (node->calls_comdat_local)
+    reason = "calls comdat-local function";
 
   if (reason && dump_file && !node->alias && !node->thunk.thunk_p)
     fprintf (dump_file, "Function %s/%i is not versionable, reason: %s.\n",
diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index da3c2c62344..8993cae089c 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -1360,6 +1360,13 @@ split_function (basic_block return_bb, struct split_point *split_point,
 
   node->split_part = true;
 
+  if (cur_node->same_comdat_group)
+    {
+      cur_node->calls_comdat_local = 1;
+      node->add_to_same_comdat_group (cur_node);
+    }
+
+
   /* Let's take a time profile for splitted function.  */
   node->tp_first_run = cur_node->tp_first_run + 1;
 
diff --git a/gcc/testsuite/g++.dg/ipa/pr80212.C b/gcc/testsuite/g++.dg/ipa/pr80212.C
new file mode 100644
index 00000000000..60d3b613035
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr80212.C
@@ -0,0 +1,18 @@
+// PR ipa/80212
+// { dg-options "-O2 --param partial-inlining-entry-probability=403796683 -fno-early-inlining" }
+
+struct b
+{
+  virtual b *c () const;
+};
+struct d : virtual b
+{
+};
+struct e : d
+{
+  e *
+  c () const
+  {
+  }
+};
+main () { e a; }
-- 
2.12.2

Re: [PATCH] Add function part to a same comdat group (PR ipa/80212).

[Jan Hubicka]

> Hello.
> 
> This is my second attempt to fix the PR I worked on with Honza and Martin Jambor.
> 
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Apart from that
> octoploid confirmed it can survive LLVM build. And I can build Firefox and boost with the
> patch on x86_64-linux-gnu.
> 
> Ready to be installed?
> Martin

> >From f8934791a3d345eb8c2c51beca07177c75e5f6ac Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Tue, 11 Apr 2017 14:22:50 +0200
> Subject: [PATCH] Add function part to a same comdat group (PR ipa/80212).
> 
> gcc/ChangeLog:
> 
> 2017-04-11  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/80212
> 	* cgraph.c (cgraph_node::dump): Dump calls_comdat_local.
> 	* ipa-cp.c (determine_versionability): Handle calls_comdat_local
> 	flags.
> 	* ipa-split.c (split_function): Create a local comdat symbol
> 	if caller is in a comdat group.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-04-11  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/80212
> 	* g++.dg/ipa/pr80212.C: New test.
> ---
>  gcc/cgraph.c                       |  2 ++
>  gcc/ipa-cp.c                       |  2 ++
>  gcc/ipa-split.c                    |  7 +++++++
>  gcc/testsuite/g++.dg/ipa/pr80212.C | 18 ++++++++++++++++++
>  4 files changed, 29 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr80212.C
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 92ae0910c60..e505b10e211 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -2123,6 +2123,8 @@ cgraph_node::dump (FILE *f)
>      fprintf (f, " only_called_at_exit");
>    if (tm_clone)
>      fprintf (f, " tm_clone");
> +  if (calls_comdat_local)
> +    fprintf (f, " calls_comdat_local");
>    if (icf_merged)
>      fprintf (f, " icf_merged");
>    if (merged_comdat)
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 0b408149a88..756a335661d 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -614,6 +614,8 @@ determine_versionability (struct cgraph_node *node,
>       decloned constructors, inlining is always better anyway.  */
>    else if (node->comdat_local_p ())
>      reason = "comdat-local function";
> +  else if (node->calls_comdat_local)
> +    reason = "calls comdat-local function";

Perhaps add a comment here that the call is versionable if we make sure that all
callers are inside of the comdat group.
We could improve it later if it becomes important.
>  
>    if (reason && dump_file && !node->alias && !node->thunk.thunk_p)
>      fprintf (dump_file, "Function %s/%i is not versionable, reason: %s.\n",
> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> index da3c2c62344..8993cae089c 100644
> --- a/gcc/ipa-split.c
> +++ b/gcc/ipa-split.c
> @@ -1360,6 +1360,13 @@ split_function (basic_block return_bb, struct split_point *split_point,
>  
>    node->split_part = true;
>  
> +  if (cur_node->same_comdat_group)
> +    {
> +      cur_node->calls_comdat_local = 1;
> +      node->add_to_same_comdat_group (cur_node);
> +    }
Also please add a comment here as well.

Otherwise OK.  I would suggest commit the first part (fixing comdat local handling) 
first and ipa-split bits incrementally to simplify bisecting we may need to do in future
(I hope we won't)

Honza