public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jan Hubicka <hubicka@ucw.cz>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Jason Merrill <jason@redhat.com>,
	Richard Biener <rguenther@suse.de>,
	Patrick Palka <ppalka@redhat.com>,
	gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]
Date: Wed, 17 Apr 2024 11:04:26 +0200	[thread overview]
Message-ID: <Zh-Qmlemu6-yk9HC@kam.mff.cuni.cz> (raw)
In-Reply-To: <Zh99dzErl4LUr2AC@tucnak>

> Hi!
Hello,
> The reason the optimization doesn't trigger when the constructor is
> constexpr is that expand_or_defer_fn is called in that case much earlier
> than when it is not constexpr; in the former case it is called when we
> try to constant evaluate that constructor.  But DECL_INTERFACE_KNOWN
> is false in that case and comdat_linkage hasn't been called either
> (I think it is desirable, because comdat group is stored in the cgraph
> node and am not sure it is a good idea to create cgraph nodes for
> something that might not be needed later on at all), so maybe_clone_body
> clones the bodies, but doesn't make them as aliases.

Thanks for working this out! Creating cgraph node with no body is
harmless as it will be removed later.  
> 
> The following patch is an attempt to redo this optimization when
> import_export_decl is called at_eof time on the base/complete cdtor
> (or deleting dtor).  It will not do anything if maybe_clone_body
> hasn't been called uyet (the TREE_ASM_WRITTEN check on the
> DECL_MAYBE_IN_CHARGE_CDTOR_P), or when one or both of the base/complete
> cdtors have been lowered already, or when maybe_clone_body called
> maybe_thunk_body and it was successful.  Otherwise retries the
> can_alias_cdtor check and makes the complete cdtor alias to the
> base cdtor with adjustments to the comdat group.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2024-04-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR lto/113208
> 	* cp-tree.h (maybe_optimize_cdtor): Declare.
> 	* decl2.cc (import_export_decl): Call it for cloned cdtors.
> 	* optimize.cc (maybe_optimize_cdtor): New function.
> 
> 	* g++.dg/lto/pr113208_0.C: New test.
> 	* g++.dg/lto/pr113208_1.C: New file.
> 	* g++.dg/lto/pr113208.h: New file.
> 
> --- gcc/cp/cp-tree.h.jj	2024-04-16 17:18:37.286279533 +0200
> +++ gcc/cp/cp-tree.h	2024-04-16 17:45:01.685635709 +0200
> @@ -7447,6 +7447,7 @@ extern bool handle_module_option (unsign
>  /* In optimize.cc */
>  extern tree clone_attrs				(tree);
>  extern bool maybe_clone_body			(tree);
> +extern void maybe_optimize_cdtor		(tree);
>  
>  /* In parser.cc */
>  extern tree cp_convert_range_for (tree, tree, tree, cp_decomp *, bool,
> --- gcc/cp/decl2.cc.jj	2024-04-16 17:18:37.287279519 +0200
> +++ gcc/cp/decl2.cc	2024-04-16 17:45:01.686635695 +0200
> @@ -3568,6 +3568,9 @@ import_export_decl (tree decl)
>      }
>  
>    DECL_INTERFACE_KNOWN (decl) = 1;
> +
> +  if (DECL_CLONED_FUNCTION_P (decl))
> +    maybe_optimize_cdtor (decl);
>  }
>  
>  /* Return an expression that performs the destruction of DECL, which
> --- gcc/cp/optimize.cc.jj	2024-04-16 17:18:37.374278327 +0200
> +++ gcc/cp/optimize.cc	2024-04-16 21:35:53.597188774 +0200
> @@ -723,3 +723,58 @@ maybe_clone_body (tree fn)
>    /* We don't need to process the original function any further.  */
>    return 1;
>  }
> +
> +/* If maybe_clone_body is called while the cdtor is still tentative,
> +   DECL_ONE_ONLY will be false and so will be can_alias_cdtor (fn).
> +   In that case we wouldn't try to optimize using an alias and instead
> +   would emit separate base and complete cdtor.  The following function
> +   attempts to still optimize that case when we import_export_decl
> +   is called first time on one of the clones.  */
> +
> +void
> +maybe_optimize_cdtor (tree orig_decl)
> +{
> +  tree fns[3];
> +  tree fn = DECL_CLONED_FUNCTION (orig_decl);
> +  gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn));
> +  if (DECL_INTERFACE_KNOWN (fn)
> +      || !TREE_ASM_WRITTEN (fn)
> +      || !DECL_ONE_ONLY (orig_decl)
> +      || symtab->global_info_ready)
> +    return;
> +
> +  populate_clone_array (fn, fns);
> +
> +  if (!fns[0] || !fns[1])
> +    return;
> +  for (int i = 2 - !fns[2]; i >= 0; --i)
> +    if (fns[i] != orig_decl && DECL_INTERFACE_KNOWN (fns[i]))
> +      return;
> +  DECL_INTERFACE_KNOWN (fn) = 1;
> +  comdat_linkage (fn);
> +  if (!can_alias_cdtor (fn))
> +    return;
> +  /* For comdat base and complete cdtors put them into the same,
> +     *[CD]5* comdat group instead of *[CD][12]*.  */
> +  auto n0 = cgraph_node::get_create (fns[0]);
> +  auto n1 = cgraph_node::get_create (fns[1]);
> +  auto n2 = fns[2] ? cgraph_node::get_create (fns[1]) : NULL;
> +  if (n0->lowered || n1->lowered || (n2 && n2->lowered))
> +    return;
> +  import_export_decl (fns[0]);
> +  n1->definition = false;
> +  if (!n0->create_same_body_alias (fns[1], fns[0]))
> +    return;
> +
> +  tree comdat_group = cdtor_comdat_group (fns[1], fns[0]);
> +  n1 = cgraph_node::get (fns[1]);
> +  n0->set_comdat_group (comdat_group);
> +  if (n1->same_comdat_group)
> +    n1->remove_from_same_comdat_group ();
> +  n1->add_to_same_comdat_group (n0);
> +  if (fns[2])
> +    n2->add_to_same_comdat_group (n0);
> +  import_export_decl (fns[1]);

So this is handling the case where symbol was already inserted into one
comdat group and later we need to move it into the C5 group?  As long as
we move everythingf rom old comdat group, this should be fine.
> +  /* Remove the body now that it is an alias.  */
> +  DECL_SAVED_TREE (fns[1]) = NULL_TREE;
Maybe using release_function_body since it also knows how to remove
DECL_STRUCT_FUNCTION that exists at this stage?

I was thinking how to solve the problem on cgrpah side.  We definitely
have long lasting bug where aliases are handled incorrectly for which I
made WIP patch (but probably would like to hold it after release branch is
created).  When foo has alias bar and foo is praviled then the alias
target is prevailed too.  This is what causes the ICE about cross comdat
section alias.  However fixing this is not enough as I do not think we
can handle incremental linking correctly (as discussed briefly on IRC
technically we should keep both sections but that would require two
symbols of same name in single .o file).

With the aliasing fixed we turn the other symbol into static but keep
alias, so we end up with one comdat group having the non-aliased
constructor and second comdat group (C5) exporting only the alias, which
is not quite reight either.

Honza

  reply	other threads:[~2024-04-17  9:04 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-17  7:42 Jakub Jelinek
2024-04-17  9:04 ` Jan Hubicka [this message]
2024-04-17 12:32   ` Jakub Jelinek
2024-04-17 13:26     ` Jan Hubicka
2024-04-17 14:07       ` Jakub Jelinek
2024-04-17 14:34         ` Jan Hubicka
2024-04-17 14:39           ` Jakub Jelinek
2024-04-22 15:42 ` [PATCH] c++, v2: " Jakub Jelinek
2024-04-23  3:14   ` Jason Merrill
2024-04-23 16:04     ` Jakub Jelinek
2024-04-24  9:16       ` Jonathan Wakely
2024-04-24 16:16         ` [PATCH] c++, v3: " Jakub Jelinek
2024-04-24 22:39           ` Jason Merrill
2024-04-24 22:47             ` Jakub Jelinek
2024-04-25  0:43               ` Jason Merrill
2024-04-25 12:02                 ` [PATCH] c++, v4: " Jakub Jelinek
2024-04-25 14:22                   ` Jakub Jelinek
2024-04-25 15:30                     ` Jason Merrill
2024-04-25 18:42                       ` [PATCH] c++, v5: " Jakub Jelinek
2024-05-09 18:20                       ` [PATCH] c++: Optimize in maybe_clone_body aliases even when not at_eof [PR113208] Jakub Jelinek
2024-05-09 18:58                         ` Marek Polacek
2024-05-09 19:05                           ` Jakub Jelinek
2024-05-10 19:59                         ` Jason Merrill
2024-05-13 10:19                           ` Jakub Jelinek
2024-05-14 22:20                             ` Jason Merrill

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=Zh-Qmlemu6-yk9HC@kam.mff.cuni.cz \
    --to=hubicka@ucw.cz \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=jason@redhat.com \
    --cc=ppalka@redhat.com \
    --cc=rguenther@suse.de \
    /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).