Re: [PATCH: PR /3187] C++ space optimization: declone delete destructor

[Richard Guenther <richard dot guenther at gmail dot com>]

On Mon, Sep 7, 2009 at 10:03 AM, Carrot Wei<carrot@google.com> wrote:
> Thank you Richard, unconditionally construct deleting destructor this way is
> also a good idea, since it can still get inlined later. I've modified the
> patch according to your suggestion. Could you take another look?
>
> I've run the boot-strap and gcc regression tests again, and without any new
> failure.

It looks good but I can't approve it.  I think we need to make sure we
have a way
of forcing that call to bind locally (otherwise inlining wouldn't work as well).
Do we ever have weak destructors?  OTOH they need to follow ODR so they'd
be comdat at most - can you verify the call is indeed inlined?

Thanks,
Richard.

> thanks
> Wei Guozhi
>
>
> Index: optimize.c
> ===================================================================
> --- optimize.c ?(revision 151143)
> +++ optimize.c ?(working copy)
> @@ -106,6 +106,41 @@ clone_body (tree clone, tree fn, void *a
> ? append_to_statement_list_force (stmts, &DECL_SAVED_TREE (clone));
> ?}
>
> +/* DELETE_DTOR is a delete destructor whose body will be build.
> + ? COMPLETE_DTOR is the corresponding complete destructor. ?*/
> +
> +static void
> +build_delete_destructor_body (tree delete_dtor, tree complete_dtor)
> +{
> + ?tree call_dtor, call_delete;
> + ?tree parm = DECL_ARGUMENTS (delete_dtor);
> + ?tree virtual_size = cxx_sizeof (current_class_type);
> +
> + ?/* Call the corresponding complete destructor. ?*/
> + ?gcc_assert (complete_dtor);
> + ?call_dtor = build_cxx_call (complete_dtor, 1, &parm);
> + ?add_stmt (call_dtor);
> +
> + ?add_stmt (build_stmt (0, LABEL_EXPR, cdtor_label));
> +
> + ?/* Call the delete function. ?*/
> + ?call_delete = build_op_delete_call(DELETE_EXPR, current_class_ptr,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? virtual_size,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*global_p=*/false,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*placement=*/NULL_TREE,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*alloc_fn=*/NULL_TREE);
> + ?add_stmt (call_delete);
> +
> + ?/* Return the address of the object. ?*/
> + ?if (targetm.cxx.cdtor_returns_this ())
> + ? ?{
> + ? ? ?tree val = DECL_ARGUMENTS (delete_dtor);
> + ? ? ?val = build2 (MODIFY_EXPR, TREE_TYPE (val),
> + ? ? ? ? ? ? ? ? ? ?DECL_RESULT (delete_dtor), val);
> + ? ? ?add_stmt (build_stmt (0, RETURN_EXPR, val));
> + ? ?}
> +}
> +
> ?/* 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. ?*/
> @@ -114,6 +149,7 @@ bool
> ?maybe_clone_body (tree fn)
> ?{
> ? tree clone;
> + ?tree complete_dtor = NULL_TREE;
> ? bool first = true;
>
> ? /* We only clone constructors and destructors. ?*/
> @@ -124,6 +160,15 @@ maybe_clone_body (tree fn)
> ? /* Emit the DWARF1 abstract instance. ?*/
> ? (*debug_hooks->deferred_inline_function) (fn);
>
> + ?/* Look for the complete destructor which may be used to build the
> + ? ? delete destructor. ?*/
> + ?FOR_EACH_CLONE (clone, fn)
> + ? ?if (DECL_NAME (clone) == complete_dtor_identifier)
> + ? ? ?{
> + ? ? ? ?complete_dtor = clone;
> + ? ? ? ?break;
> + ? ? ?}
> +
> ? /* We know that any clones immediately follow FN in the TYPE_METHODS
> ? ? ?list. ?*/
> ? push_to_top_level ();
> @@ -224,8 +269,14 @@ maybe_clone_body (tree fn)
> ? ? ? ? ?clone_parm = DECL_RESULT (clone);
> ? ? ? ? ?*pointer_map_insert (decl_map, parm) = clone_parm;
> ? ? ? ?}
> - ? ? ?/* Clone the body. ?*/
> - ? ? ?clone_body (clone, fn, decl_map);
> +
> + ? ? ?/* Build the delete destructor by calling complete destructor
> + ? ? ? ? and delete function. ?*/
> + ? ? ?if (DECL_NAME (clone) == deleting_dtor_identifier)
> + ? ? ? ?build_delete_destructor_body (clone, complete_dtor);
> + ? ? ?else
> + ? ? ? ?/* Clone the body. ?*/
> + ? ? ? ?clone_body (clone, fn, decl_map);
>
> ? ? ? /* Clean up. ?*/
> ? ? ? pointer_map_destroy (decl_map);
>
>
> On Thu, Sep 3, 2009 at 10:20 PM, Richard
> Guenther<richard.guenther@gmail.com> wrote:
>> On Thu, Sep 3, 2009 at 4:04 PM, Carrot Wei<carrot@google.com> wrote:
>>> This patch fixes a small part of
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=3187
>>>
>>> The delete destructor(D0) always contains the content of complete
>>> desturctor(D1) followed by a function call to delete. So instead of cloning
>>> the abstract destructor function body to the delete destructor(D0), we
>>> can generate a function call to complete destructor(D1) followed by a function
>>> call to delete.
>>>
>>> This method doesn't break the ABI or linker compatibility.
>>>
>>> With this patch we can reduce 2.59% instructions for an open source 2d library
>>> skia on arm. Some function call to complete destructor(D1) will be inlined in
>>> later pass. So more instructions can be removed if we can do better inlining.
>>>
>>> Test:
>>> Bootstrapped on linux-x86.
>>> gcc regression tests without new failure.
>>>
>>>
>>> ChangeLog:
>>> 2009-09-03 ?Wei Guozhi ?<carrot@google.com>
>>>
>>> ? ? ? ?PR /3187
>>> ? ? ? ?* cp/optimize.c (build_delete_destructor_body): New function.
>>> ? ? ? ?(maybe_clone_body): Call build_delete_destructor_body if optimize for
>>> ? ? ? ?size.
>>>
>>> thanks
>>> Guozhi
>>>
>>>
>>>
>>> Index: optimize.c
>>> ===================================================================
>>> --- optimize.c ?(revision 151143)
>>> +++ optimize.c ?(working copy)
>>> @@ -106,6 +106,41 @@ clone_body (tree clone, tree fn, void *a
>>> ? append_to_statement_list_force (stmts, &DECL_SAVED_TREE (clone));
>>> ?}
>>>
>>> +/* DELETE_DTOR is a delete destructor whose body will be build.
>>> + ? COMPLETE_DTOR is the corresponding complete destructor. ?*/
>>> +
>>> +static void
>>> +build_delete_destructor_body (tree delete_dtor, tree complete_dtor)
>>> +{
>>> + ?tree call_dtor, call_delete;
>>> + ?tree parm = DECL_ARGUMENTS (delete_dtor);
>>> + ?tree virtual_size = cxx_sizeof (current_class_type);
>>> +
>>> + ?/* Call the corresponding complete destructor. ?*/
>>> + ?gcc_assert (complete_dtor);
>>> + ?call_dtor = build_cxx_call (complete_dtor, 1, &parm);
>>> + ?add_stmt (call_dtor);
>>> +
>>> + ?add_stmt (build_stmt (0, LABEL_EXPR, cdtor_label));
>>> +
>>> + ?/* Call the delete function. ?*/
>>> + ?call_delete = build_op_delete_call(DELETE_EXPR, current_class_ptr,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? virtual_size,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*global_p=*/false,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*placement=*/NULL_TREE,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*alloc_fn=*/NULL_TREE);
>>> + ?add_stmt (call_delete);
>>> +
>>> + ?/* Return the address of the object. ?*/
>>> + ?if (targetm.cxx.cdtor_returns_this ())
>>> + ? ?{
>>> + ? ? ?tree val = DECL_ARGUMENTS (delete_dtor);
>>> + ? ? ?val = build2 (MODIFY_EXPR, TREE_TYPE (val),
>>> + ? ? ? ? ? ? ? ? ? ?DECL_RESULT (delete_dtor), val);
>>> + ? ? ?add_stmt (build_stmt (0, RETURN_EXPR, val));
>>> + ? ?}
>>> +}
>>> +
>>> ?/* 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. ?*/
>>> @@ -114,6 +149,7 @@ bool
>>> ?maybe_clone_body (tree fn)
>>> ?{
>>> ? tree clone;
>>> + ?tree complete_dtor = NULL_TREE;
>>> ? bool first = true;
>>>
>>> ? /* We only clone constructors and destructors. ?*/
>>> @@ -124,6 +160,15 @@ maybe_clone_body (tree fn)
>>> ? /* Emit the DWARF1 abstract instance. ?*/
>>> ? (*debug_hooks->deferred_inline_function) (fn);
>>>
>>> + ?/* Look for the complete destructor which may be used to build the
>>> + ? ? delete destructor. ?*/
>>> + ?FOR_EACH_CLONE (clone, fn)
>>> + ? ?if (DECL_NAME (clone) == complete_dtor_identifier)
>>> + ? ? ?{
>>> + ? ? ? ?complete_dtor = clone;
>>> + ? ? ? ?break;
>>> + ? ? ?}
>>> +
>>> ? /* We know that any clones immediately follow FN in the TYPE_METHODS
>>> ? ? ?list. ?*/
>>> ? push_to_top_level ();
>>> @@ -224,8 +269,15 @@ maybe_clone_body (tree fn)
>>> ? ? ? ? ?clone_parm = DECL_RESULT (clone);
>>> ? ? ? ? ?*pointer_map_insert (decl_map, parm) = clone_parm;
>>> ? ? ? ?}
>>> - ? ? ?/* Clone the body. ?*/
>>> - ? ? ?clone_body (clone, fn, decl_map);
>>> +
>>> + ? ? ?/* If we are optimizing for size, we can build the delete destructor
>>> + ? ? ? ? by calling complete destructor and delete function. ?*/
>>> + ? ? ?if ((DECL_NAME (clone) == deleting_dtor_identifier)
>>> + ? ? ? ? ?&& optimize_function_for_size_p (DECL_STRUCT_FUNCTION (clone)))
>>> + ? ? ? ?build_delete_destructor_body (clone, complete_dtor);
>>
>> Using optimize_function_for_size_p doens't make sense in the frontends.
>> Just use optimize_size here. ?In fact I would suggest to unconditionally
>> go with the build_delete_destructor_body variant.
>>
>> In future we might want to have a way to mark this call as always binding
>> locally to avoid a relocation here and allow better optimization.
>>
>> Richard.
>>
>>> + ? ? ?else
>>> + ? ? ? ?/* Clone the body. ?*/
>>> + ? ? ? ?clone_body (clone, fn, decl_map);
>>>
>>> ? ? ? /* Clean up. ?*/
>>> ? ? ? pointer_map_destroy (decl_map);
>>>
>>
>