PING^1
On 7/11/19 8:45 AM, Martin Liška wrote:
> On 7/9/19 11:00 PM, Jason Merrill wrote:
>> On 7/9/19 6:17 AM, Marc Glisse wrote:
>>> On Tue, 9 Jul 2019, Martin Liška wrote:
>>>
>>>> On 7/9/19 9:49 AM, Marc Glisse wrote:
>>>>> On Tue, 9 Jul 2019, Marc Glisse wrote:
>>>>>
>>>>>> On Mon, 8 Jul 2019, Martin Liška wrote:
>>>>>>
>>>>>>>> The patch apparently has DECL_IS_OPERATOR_DELETE only on the
>>>>>>>> replaceable global deallocation functions, not all delete operators,
>>>>>>>> contrary to DECL_IS_OPERATOR_NEW, so the name is misleading. On the
>>>>>>>> other hand, those seem to be the ones for which the optimization is
>>>>>>>> legal (well, not quite, the rules are in terms of operator new, and I
>>>>>>>> am not sure how well operator delete has to match, but close enough).
>>>>>>>
>>>>>>> Are you talking about this location where we set OPERATOR_NEW:
>>>>>>> https://github.com/gcc-mirror/gcc/blob/master/gcc/cp/decl.c#L13643
>>>>>>> ?
>>>>>>>
>>>>>>> That's the only place where we set OPERATOR_NEW flag and not
>>>>>>> OPERATOR_DELETE.
>>>>>>
>>>>>> Yes, I think that's the place.
>>>>>>
>>>>>> Again, not setting DECL_IS_OPERATOR_DELETE on local operator delete
>>>>>> seems misleading, but setting it would let us optimize in cases where we
>>>>>> are not really allowed to. Maybe just rename your macro to
>>>>>> DECL_IS_GLOBAL_OPERATOR_DELETE?
>>>>>
>>>>> Hmm, I replied too fast.
>>>>>
>>>>> Global operator delete does not seem like a good terminology, the ones
>>>>> marked in the patch would be the usual (=non-placement) replaceable
>>>>> deallocation functions.
>>>>>
>>>>> I cannot find a requirement that operator new and operator delete should
>>>>> match. The rules to omit allocation are stated in terms of which operator
>>>>> new is called, but do not seem to care which operator delete is used. So
>>>>> allocating with the global operator new and deallocating with a class
>>>>> overload of operator delete can be removed, but not the reverse (not sure
>>>>> how they came up with such a rule...).
>>
>> Correct. The standard just says that an implementation is allowed to omit a
>> call to the replaceable ::operator new; it does not place any constraints on
>> that, the conditions for such omission are left up to the implementation.
>>
>> If the user's code uses global new and class delete for the same pointer,
>> that would suggest that they're doing something odd, and we might as well
>> leave it alone. I would expect this to be very rare.
>>
>>>>> Which means we would need:
>>>>
>>>> Thank you Mark for digging deep in that.
>>>>
>>>>>
>>>>> keep DECL_IS_OPERATOR_NEW for the current uses
>>>>>
>>>>> DECL_IS_REPLACEABLE_OPERATOR_NEW (equivalent to DECL_IS_OPERATOR_NEW &&
>>>>> DECL_IS_MALLOC? not exactly but close I think) for DCE
>>>>>
>>>>> DECL_IS_OPERATOR_DELETE (which also includes some class overloads) for DCE
>>>>
>>>> Note that with the current version of the patch we are out of free bits in
>>>> struct GTY(()) tree_function_decl.
>>>> Would it be possible to tweak the current patch to cover what you
>>>> described?
>>>
>>> If you approximate DECL_IS_REPLACEABLE_OPERATOR_NEW with
>>> DECL_IS_OPERATOR_NEW && DECL_IS_MALLOC, it shouldn't need more bits than
>>> the current patch. I think the main difference is if a user adds attribute
>>> malloc to his class-specific operator new, where it will enable DCE, but
>>> since the attribute is non-standard, we can just document that behavior, it
>>> might even be desirable.
>>
>> Sure, it seems desirable to me.
>>
>> Jason
>
> Ok, I hopefully addressed the suggested improvements to the patch.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
