[PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jakub Jelinek]

Hi!

This patch on top of the patch I've just sent also optimizes comdat
cdtors if HAVE_COMDAT_GROUP and the symbols are weak.
If the cdtors have the same body, we always force both the function and
alias to be created whenever one of them is needed, and use a different
comdat group name *C.* instead of *C1* and *C2* (and similarly for D[.12]).
When not mixing object files from different compilers we save space, if we
mix files from different compilers we risk that both this *C.* comdat group
will stay and also both the *C1* and *C2* from other compiler.
To avoid that we'd need to invent a linker extension.

I'm posting this separately from the patch because Jason wanted to discuss
this on the ABI list first.

Also, this patch regresses abi_check, libstdc++.so.6 now exports with this
patch some symbols it wasn't exporting before:
_ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEED2Ev@@GLIBCXX_3.4.10 FUNC WEAK DEFAULT
_ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEED2Ev@@GLIBCXX_3.4.10 FUNC WEAK DEFAULT
_ZNSt11range_errorD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt12domain_errorD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt12length_errorD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt12out_of_rangeD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt14overflow_errorD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt15basic_stringbufIcSt11char_traitsIcESaIcEED2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt15basic_stringbufIwSt11char_traitsIwESaIwEED2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt15underflow_errorD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
_ZNSt16invalid_argumentD2Ev@@GLIBCXX_3.4 FUNC WEAK DEFAULT
the symbol version script would need to be tightened up not to export those.

2009-11-13  Jakub Jelinek  <jakub@redhat.com>

	PR c++/3187
	* optimize.c (cdtor_comdat_group): New function.
	(maybe_clone_body): Also optimize DECL_COMDAT base/complete cdtors.

--- gcc/cp/optimize.c.jj2	2009-11-13 14:51:21.000000000 +0100
+++ gcc/cp/optimize.c	2009-11-13 12:55:47.000000000 +0100
@@ -142,6 +142,45 @@ build_delete_destructor_body (tree delet
     }
 }
 
+/* Return name of comdat group for complete and base ctor (or dtor)
+   that have the same body.  */
+
+static tree
+cdtor_comdat_group (tree complete, tree base)
+{
+  tree complete_name = DECL_COMDAT_GROUP (complete);
+  tree base_name = DECL_COMDAT_GROUP (base);
+  char *grp_name;
+  const char *p, *q;
+  bool diff_seen = false;
+  size_t idx;
+  if (complete_name == NULL)
+    complete_name = cxx_comdat_group (complete);
+  if (base_name == NULL)
+    base_name = cxx_comdat_group (base);
+  gcc_assert (IDENTIFIER_LENGTH (complete_name)
+	      == IDENTIFIER_LENGTH (base_name));
+  grp_name = XALLOCAVEC (char, IDENTIFIER_LENGTH (complete_name) + 1);
+  p = IDENTIFIER_POINTER (complete_name);
+  q = IDENTIFIER_POINTER (base_name);
+  for (idx = 0; idx < IDENTIFIER_LENGTH (complete_name); idx++)
+    if (p[idx] == q[idx])
+      grp_name[idx] = p[idx];
+    else
+      {
+	gcc_assert (!diff_seen
+		    && idx > 0
+		    && (p[idx - 1] == 'C' || p[idx - 1] == 'D')
+		    && p[idx] == '1'
+		    && q[idx] == '2');
+	grp_name[idx] = '.';
+	diff_seen = true;
+      }
+  grp_name[idx] = '\0';
+  gcc_assert (diff_seen);
+  return get_identifier (grp_name);
+}
+
 /* FN is a function that has a complete body.  Clone the body as
    necessary.  Returns nonzero if there's no longer any need to
    process the main body.  */
@@ -243,9 +282,18 @@ maybe_clone_body (tree fn)
 	  if (seen == -1)
 	    seen = fns[0] == clone ? 0 : 1;
 	  else if (DECL_INTERFACE_KNOWN (fns[seen])
-		   && !DECL_ONE_ONLY (fns[seen])
 		   && cgraph_same_body_alias (clone, fns[seen]))
-	    alias = true;
+	    {
+	      alias = true;
+	      if (DECL_ONE_ONLY (fns[seen])
+		  && DECL_WEAK (fns[seen])
+		  && HAVE_COMDAT_GROUP)
+		{
+		  DECL_COMDAT_GROUP (fns[0])
+		    = cdtor_comdat_group (fns[0], fns[1]);
+		  DECL_COMDAT_GROUP (fns[1]) = DECL_COMDAT_GROUP (fns[0]);
+		}
+	    }
 	}
 
       /* Build the delete destructor by calling complete destructor

	Jakub

[PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187, take 2)

[Jakub Jelinek]

Hi!

On Fri, Nov 13, 2009 at 07:07:08PM +0100, Jakub Jelinek wrote:
> This patch on top of the patch I've just sent also optimizes comdat
> cdtors if HAVE_COMDAT_GROUP and the symbols are weak.
> If the cdtors have the same body, we always force both the function and
> alias to be created whenever one of them is needed, and use a different
> comdat group name *C.* instead of *C1* and *C2* (and similarly for D[.12]).
> When not mixing object files from different compilers we save space, if we
> mix files from different compilers we risk that both this *C.* comdat group
> will stay and also both the *C1* and *C2* from other compiler.
> To avoid that we'd need to invent a linker extension.
> 
> I'm posting this separately from the patch because Jason wanted to discuss
> this on the ABI list first.

Here is an updated patch on top of the patch I've just sent,
bootstrapped/regtested on x86_64-linux and i686-linux.
The same caveats apply (libstdc++ exports need small tweaking to avoid
abi_check FAIL).

2009-11-16  Jakub Jelinek  <jakub@redhat.com>

	PR c++/3187
	* optimize.c (cdtor_comdat_group): New function.
	(maybe_clone_body): Also optimize DECL_COMDAT base/complete cdtors.

	* g++.dg/abi/mangle1.C: Allow _ZN1AC2Ev and _ZN1BC2Ev to be
	emitted as aliases to _ZN1AC1Ev resp. _ZN1BC2Ev.

--- gcc/cp/optimize.c.jj	2009-11-16 11:38:00.000000000 +0100
+++ gcc/cp/optimize.c	2009-11-16 13:43:40.000000000 +0100
@@ -142,6 +142,45 @@ build_delete_destructor_body (tree delet
     }
 }
 
+/* Return name of comdat group for complete and base ctor (or dtor)
+   that have the same body.  */
+
+static tree
+cdtor_comdat_group (tree complete, tree base)
+{
+  tree complete_name = DECL_COMDAT_GROUP (complete);
+  tree base_name = DECL_COMDAT_GROUP (base);
+  char *grp_name;
+  const char *p, *q;
+  bool diff_seen = false;
+  size_t idx;
+  if (complete_name == NULL)
+    complete_name = cxx_comdat_group (complete);
+  if (base_name == NULL)
+    base_name = cxx_comdat_group (base);
+  gcc_assert (IDENTIFIER_LENGTH (complete_name)
+	      == IDENTIFIER_LENGTH (base_name));
+  grp_name = XALLOCAVEC (char, IDENTIFIER_LENGTH (complete_name) + 1);
+  p = IDENTIFIER_POINTER (complete_name);
+  q = IDENTIFIER_POINTER (base_name);
+  for (idx = 0; idx < IDENTIFIER_LENGTH (complete_name); idx++)
+    if (p[idx] == q[idx])
+      grp_name[idx] = p[idx];
+    else
+      {
+	gcc_assert (!diff_seen
+		    && idx > 0
+		    && (p[idx - 1] == 'C' || p[idx - 1] == 'D')
+		    && p[idx] == '1'
+		    && q[idx] == '2');
+	grp_name[idx] = '.';
+	diff_seen = true;
+      }
+  grp_name[idx] = '\0';
+  gcc_assert (diff_seen);
+  return get_identifier (grp_name);
+}
+
 /* FN is a function that has a complete body.  Clone the body as
    necessary.  Returns nonzero if there's no longer any need to
    process the main body.  */
@@ -248,9 +287,18 @@ maybe_clone_body (tree fn)
 	  && idx == 1
 	  && !flag_use_repository
 	  && DECL_INTERFACE_KNOWN (fns[0])
-	  && !DECL_ONE_ONLY (fns[0])
 	  && cgraph_same_body_alias (clone, fns[0]))
-	alias = true;
+	{
+	  alias = true;
+	  if (DECL_ONE_ONLY (fns[0])
+	      && DECL_WEAK (fns[0])
+	      && HAVE_COMDAT_GROUP)
+	    {
+	      DECL_COMDAT_GROUP (fns[0])
+		= cdtor_comdat_group (fns[0], fns[1]);
+	      DECL_COMDAT_GROUP (fns[1]) = DECL_COMDAT_GROUP (fns[0]);
+	    }
+	}
 
       /* Build the delete destructor by calling complete destructor
          and delete function.  */
--- gcc/testsuite/g++.dg/abi/mangle1.C.jj	2008-09-30 16:55:21.000000000 +0200
+++ gcc/testsuite/g++.dg/abi/mangle1.C	2009-11-16 15:34:22.000000000 +0100
@@ -13,8 +13,8 @@ struct C: public B { };
 C c;
 
 // { dg-final { scan-assembler "\n_?_ZN1A1fEv\[: \t\n\]" } }
-// { dg-final { scan-assembler "\n_?_ZN1AC2Ev\[: \t\n\]" } }
-// { dg-final { scan-assembler "\n_?_ZN1BC2Ev\[: \t\n\]" } }
+// { dg-final { scan-assembler "\n_?_ZN1AC\[12\]Ev\[: \t\n\]" } }
+// { dg-final { scan-assembler "\n_?_ZN1BC\[12\]Ev\[: \t\n\]" } }
 // { dg-final { scan-assembler "\n_?_ZN1CC1Ev\[: \t\n\]" } }
 // { dg-final { scan-assembler "\n_?_ZTC1C0_1B\[: \t\n\]" } }
 // { dg-final { scan-assembler "\n_?_ZTI1A\[: \t\n\]" } }


	Jakub

Re: [PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jason Merrill]

Remind me why we can't just leave them in the same COMDAT groups they 
have now?  Is there a problem with an alias from one group to another, 
or is the concern about one compiler choosing to alias 1 to 2 and 
another compiler doing the reverse?

Jason

Re: [PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jakub Jelinek]

On Tue, Nov 17, 2009 at 05:18:08PM -0500, Jason Merrill wrote:
> Remind me why we can't just leave them in the same COMDAT groups they  
> have now?  Is there a problem with an alias from one group to another,  
> or is the concern about one compiler choosing to alias 1 to 2 and  
> another compiler doing the reverse?

There is no such thing as alias from one section to another, alias in ELF
is just another symbol for the same thing as certain symbol, so it always
has the same section and value as the other symbol.
We used to emit
_ZN1CD1Ev symbol in .text._ZN1CD1Ev section in _ZN1CD1Ev comdat group and
_ZN1CD2Ev symbol in .text._ZN1CD2Ev section in _ZN1CD2Ev comdat group,
furthermore not all CUs emitted both.
Now, if we know both functions have the same body, we'd like to emit just
_ZN1CD2Ev and
.set    _ZN1CD1Ev,_ZN1CD2Ev
If we emit _ZN1CD2Ev in .text._ZN1CD2Ev section in _ZN1CD2Ev comdat group,
it means the _ZN1CD1Ev alias is also in .text._ZN1CD2Ev section in _ZN1CD2Ev
comdat group.  We then either don't emit _ZN1CD1Ev comdat group at all,
or emit it empty.

If we emit it empty (where empty would mean the comdat group contains
empty .text._ZN1CD1Ev section):
  a.o let be g++ 4.3 compiled object that has just _ZN1CD2Ev comdat group
  (with just _ZN1CD2Ev symbol), b.o let be g++ 4.5 compiled object with
  _ZN1CD2Ev comdat group containing _ZN1CD2Ev symbol and _ZN1CD1Ev alias to
  it and empty _ZN1CD1Ev comdat group, c.o let be g++ 4.3 compiled object
  that has just _ZN1CD1Ev comdat group (with just _ZN1CD1Ev symbol).
  g++ -o prg a.o b.o c.o would mean _ZN1CD2Ev group is picked from
  a.o (thus defines _ZN1CD2Ev symbol), _ZN1CD1Ev group is picked from
  b.o (empty section), nothing is picked from c.o.  If both
  _ZN1CD1Ev and _ZN1CD2Ev are needed, this doesn't link.

If we don't emit the second comdat group at all:
  Let a.o be as above, d.o be g++ 4.5 compiled object
  with just _ZN1CD2Ev comdat group, containing both aliases, but where
  actually only _ZN1CD1Ev symbol is needed by that object.
  g++ -o prg a.o d.o would mean _ZN1CD2Ev is taken from a.o, not d.o and
  thus _ZN1CD1Ev symbol isn't defined.

Both the above cases assume that in g++ 4.5 we always emit both aliases,
even when only one of them is needed.  But if we don't do that, it means
that we hardly ever save any .text bytes.  The only case would be when all
of CUs either don't need any of _ZN1CD1Ev/_ZN1CD2Ev, or need both of them.
That's quite unlikely in a larger program.

	Jakub

Re: [PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jason Merrill]

On 11/18/2009 04:39 AM, Jakub Jelinek wrote:
> There is no such thing as alias from one section to another, alias in ELF
> is just another symbol for the same thing as certain symbol, so it always
> has the same section and value as the other symbol.

Ah, right.

What if we just make one jump to the other?

Jason

Re: [PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jakub Jelinek]

On Wed, Nov 18, 2009 at 11:24:18AM -0500, Jason Merrill wrote:
> On 11/18/2009 04:39 AM, Jakub Jelinek wrote:
>> There is no such thing as alias from one section to another, alias in ELF
>> is just another symbol for the same thing as certain symbol, so it always
>> has the same section and value as the other symbol.
>
> Ah, right.
>
> What if we just make one jump to the other?

That is only possible on some targets (the ones that allow to do a tail call
to that kind of function), on other targets you'd need to call it and return
and in case of ... arguments just give up.  It would also be more expensive
at runtime, the call takes some cycles.

But most importantly, the jump/call would essentially become part of the
ABI.  Because if say G++ 4.5 decides it jumps from *D1* to *D2* and some other
compiler decides to jump from *D2* to *D1*, then if the linker chooses
*D1* comdat group compiled by G++ 4.5 and *D2* comdat group compiled by the
* other compiler, those routines will jump to each other in an endless loop.

	Jakub

Re: [PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jason Merrill]

On 11/18/2009 01:03 PM, Jakub Jelinek wrote:
> On Wed, Nov 18, 2009 at 11:24:18AM -0500, Jason Merrill wrote:

>> What if we just make one jump to the other?
>
> That is only possible on some targets (the ones that allow to do a tail call
> to that kind of function), on other targets you'd need to call it and return
> and in case of ... arguments just give up.

Not all targets have unconditional branches?

> It would also be more expensive at runtime, the call takes some cycles.

Indeed.  One possibility would be for callers to switch to using the 
primary symbol since we know they're identical; then we wouldn't need to 
emit a definition of the other one at all.

> But most importantly, the jump/call would essentially become part of the
> ABI.  Because if say G++ 4.5 decides it jumps from *D1* to *D2* and some other
> compiler decides to jump from *D2* to *D1*, then if the linker chooses
> *D1* comdat group compiled by G++ 4.5 and *D2* comdat group compiled by the
> * other compiler, those routines will jump to each other in an endless loop.

Yes, I figured that the choice of which is the main one would go into 
the ABI.

Jason

Re: [PATCH] Also optimize DECL_ONE_ONLY cdtors (PR c++/3187)

[Jakub Jelinek]

On Wed, Nov 18, 2009 at 01:10:32PM -0500, Jason Merrill wrote:
> On 11/18/2009 01:03 PM, Jakub Jelinek wrote:
>> On Wed, Nov 18, 2009 at 11:24:18AM -0500, Jason Merrill wrote:
>
>>> What if we just make one jump to the other?
>>
>> That is only possible on some targets (the ones that allow to do a tail call
>> to that kind of function), on other targets you'd need to call it and return
>> and in case of ... arguments just give up.
>
> Not all targets have unconditional branches?

They do, but don't support e.g. jumping between functions with different TOC
etc.

>> It would also be more expensive at runtime, the call takes some cycles.
>
> Indeed.  One possibility would be for callers to switch to using the  
> primary symbol since we know they're identical; then we wouldn't need to  
> emit a definition of the other one at all.

If we switch callers, then we probably don't need any changes on the
definition side, we can keep it as is.

I'm now trying:
--- gcc/cp/call.c.jj	2009-11-18 15:48:50.000000000 +0100
+++ gcc/cp/call.c	2009-11-18 21:13:32.000000000 +0100
@@ -6044,6 +6044,14 @@ build_special_member_call (tree instance
 
   gcc_assert (instance != NULL_TREE);
 
+  if (!CLASSTYPE_VBASECLASSES (class_type))
+    {
+      if (name == base_ctor_identifier)
+	name = complete_ctor_identifier;
+      else if (name == base_dtor_identifier)
+	name = complete_dtor_identifier;
+    }
+
   fns = lookup_fnfields (binfo, name, 1);
 
   /* When making a call to a constructor or destructor for a subobject

(will of course add a comment).  This didn't regress check-g++ except for
one mangle*.C test which would need adjustment, trying to build libstdc++
now.

	Jakub