Re: [PATCH] c++, v2: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]

[Jason Merrill <jason@redhat.com>]

On 4/22/24 08:42, Jakub Jelinek wrote:
> On Wed, Apr 17, 2024 at 09:42:47AM +0200, Jakub Jelinek wrote:
>> When expand_or_defer_fn is called at_eof time, it calls import_export_decl
>> and then maybe_clone_body, which uses DECL_ONE_ONLY and comdat name in a
>> couple of places to try to optimize cdtors which are known to have the
>> same body by making the complete cdtor an alias to base cdtor (and in
>> that case also uses *[CD]5* as comdat group name instead of the normal
>> comdat group names specific to each mangled name).
>> Now, this optimization depends on DECL_ONE_ONLY and DECL_INTERFACE_KNOWN,
>> maybe_clone_body and can_alias_cdtor use:
>>        if (DECL_ONE_ONLY (fn))
>>          cgraph_node::get_create (clone)->set_comdat_group (cxx_comdat_group (clone));
>> ...
>>    bool can_alias = can_alias_cdtor (fn);
>> ...
>>        /* Tell cgraph if both ctors or both dtors are known to have
>>           the same body.  */
>>        if (can_alias
>>            && fns[0]
>>            && idx == 1
>>            && cgraph_node::get_create (fns[0])->create_same_body_alias
>>                 (clone, fns[0]))
>>          {
>>            alias = true;
>>            if (DECL_ONE_ONLY (fns[0]))
>>              {
>>                /* For comdat base and complete cdtors put them
>>                   into the same, *[CD]5* comdat group instead of
>>                   *[CD][12]*.  */
>>                comdat_group = cdtor_comdat_group (fns[1], fns[0]);
>>                cgraph_node::get_create (fns[0])->set_comdat_group (comdat_group);
>>                if (symtab_node::get (clone)->same_comdat_group)
>>                  symtab_node::get (clone)->remove_from_same_comdat_group ();
>>                symtab_node::get (clone)->add_to_same_comdat_group
>>                  (symtab_node::get (fns[0]));
>>              }
>>          }
>> and
>>    /* Don't use aliases for weak/linkonce definitions unless we can put both
>>       symbols in the same COMDAT group.  */
>>    return (DECL_INTERFACE_KNOWN (fn)
>>            && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn))
>>            && (!DECL_ONE_ONLY (fn)
>>                || (HAVE_COMDAT_GROUP && DECL_WEAK (fn))));
>> The following testcase regressed with Marek's r14-5979 change,
>> when pr113208_0.C is compiled where the ctor is marked constexpr,
>> we no longer perform this optimization, where
>> _ZN6vectorI12QualityValueEC2ERKS1_ was emitted in the
>> _ZN6vectorI12QualityValueEC5ERKS1_ comdat group and
>> _ZN6vectorI12QualityValueEC1ERKS1_ was made an alias to it,
>> instead we emit _ZN6vectorI12QualityValueEC2ERKS1_ in
>> _ZN6vectorI12QualityValueEC2ERKS1_ comdat group and the same
>> content _ZN6vectorI12QualityValueEC1ERKS1_ as separate symbol in
>> _ZN6vectorI12QualityValueEC1ERKS1_ comdat group.

This seems like an ABI bug that could use a non-LTO testcase.

>> Now, the linker seems to somehow cope with that, eventhough it
>> probably keeps both copies of the ctor, but seems LTO can't cope
>> with that and Honza doesn't know what it should do in that case
>> (linker decides that the prevailing symbol is
>> _ZN6vectorI12QualityValueEC2ERKS1_ (from the
>> _ZN6vectorI12QualityValueEC2ERKS1_ comdat group) and
>> _ZN6vectorI12QualityValueEC1ERKS1_ alias (from the other TU,
>> from _ZN6vectorI12QualityValueEC5ERKS1_ comdat group)).
>>
>> Note, the case where some constructor is marked constexpr in one
>> TU and not in another one happens pretty often in libstdc++ when
>> one mixes -std= flags used to compile different compilation units.
>>
>> 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.

Hmm, cloning the bodies and then discarding them later seems like more 
extra work than creating the cgraph nodes.

Jason