Bug 110137 - implement clang -fassume-sane-operator-new
Summary: implement clang -fassume-sane-operator-new
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 14.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2023-06-06 08:15 UTC by Richard Biener
Modified: 2024-06-30 01:22 UTC (History)
10 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-06-09 00:00:00


Attachments
Proposed implementation. (821 bytes, patch)
2024-05-27 03:07 UTC, user202729
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2023-06-06 08:15:58 UTC
clang seems to have -fassume-sane-operator-new which allows to assert that
a call to the operator has no side-effects besides the allocation, in particular that it doesn't modify or inspect global memory.
Comment 1 Richard Biener 2023-06-06 08:17:25 UTC
Related is the __builtin_operator_new (sp?) feature which possibly has the
same effect and can be used in the standard library even when
-fno-assume-sane-operator-new is in effect.

It seems clang makes -fassume-sane-operator-new the default btw.
Comment 2 Jonathan Wakely 2023-06-09 12:22:18 UTC
This would be very nice, confirmed.
Comment 3 AK 2023-08-02 04:20:07 UTC
1. clang also has noalias on nothrow versions of operator new. will `-fassume-sane-operator-new` enable that as well?

2. as per: http://eel.is/c++draft/basic.stc.dynamic#allocation-2 

"""If the request succeeds, the value returned by a replaceable allocation function is a non-null pointer value ([basic.compound]) p0 different from any previously returned value p1, unless that value p1 was subsequently passed to a replaceable deallocation function."""

Does this mean that all successful new allocations can be assumed to be a noalias as long as the pointer wasn't passed to a deallocation function? In that case when possible, can the compiler `infer` from a bottom-up analysis that an allocation is a noalias?
Comment 4 Jonathan Wakely 2023-08-02 06:56:49 UTC
Why would that depend on this new option? It's a requirement of the standard, it has to be true always.
Comment 5 rguenther@suse.de 2023-08-02 08:24:02 UTC
On Wed, 2 Aug 2023, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137
> 
> --- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> Why would that depend on this new option? It's a requirement of the standard,
> it has to be true always.

And GCC already assumes this.  Note we don't make an exception for
pointers passed to deallocation - we're using the info mainly
for memory access disambiguation and rely on the deallocation call
itself to be a barrier for code motion here.
Comment 6 user202729 2024-05-27 03:07:38 UTC
Created attachment 58293 [details]
Proposed implementation.

I've implement it. (Attached patch) It would be helpful if someone can review it.

Some notes:

1. Clang's documentation says the effect of `-fassume-sane-operator-new` is to assume no-alias, but it is pointed out that Clang also assumes no-side-effect. So I explicitly state that in the documentation.
2. The original intent is to fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110035 .

While this patch alone is insufficient to fix that (as explained in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110035#c17 ), it would be sufficient to improve the performance of the following (to only load from global memory once):

```
int a;

// Prevent optimization
void sink(void *m) {
    asm volatile("" : : "g"(m) : "memory");
}

int f(){
	int x=a;
	float* z=new float;
	int y=a;
	int result=x+y;
	sink(z);
	return result;
}
```

3. I'm not sure which additional optimizations can be done if we know something is pure, but this would improve that one case. Other cases can be added later on.
Comment 7 Jonathan Wakely 2024-05-27 07:47:34 UTC
Thanks for the patch, but patch review happens on the mailing list, not in bugzilla. Please repost to gcc-patches as documented in the submission guidelines, thanks.
Comment 8 user202729 2024-06-04 10:16:54 UTC
(In reply to Jonathan Wakely from comment #7)
> Thanks for the patch, but patch review happens on the mailing list, not in
> bugzilla. Please repost to gcc-patches as documented in the submission
> guidelines, thanks.

Thanks for letting me know, I didn't notice.

It would be helpful if you can take a look at the patch: https://gcc.gnu.org/pipermail/gcc-patches/2024-May/652977.html and let me know if it's correctly formatted.

Thank you.
Comment 9 Jan Hubicka 2024-06-04 10:39:40 UTC
Doing global flag has a problem since with LTO or using optimize
attribute user may mix code compiled with and without sane operator new.  
When function with insane operator new gets inlined to a function with
sane, we will assume sanity for calls that technically may not be sane.

So I think we do need to tag the declaration or call instead of using
flag_sane_operator_new in middle-end...
Comment 10 Jonathan Wakely 2024-06-04 11:31:25 UTC
But replacing operator new is a global property of the program. It seems to me that any translation unit claiming that operator new is sane must imply that it's sane globally.

It doesn't make sense for a function assuming A is true to be inlined into a function assuming A is false, when A will actually be the same for both.
Comment 11 Jonathan Wakely 2024-06-04 11:37:33 UTC
If a program really does need to ensure a particular TU assumes new can modify global memory (e.g. in the TU defining operator new, which makes use of some data structure) then that TU should probably be firewalled from the rest of the program, and not use LTO.
Comment 12 Jakub Jelinek 2024-06-04 11:45:35 UTC
Is the option supposed to be only about the standard global scope operator new/delete (_Znam etc.) or also user operator new/delete class methods?  If the former, then I agree it is a global property (or at least a per shared library/binary property, one can arrange stuff with symbol visibility etc.).
Otherwise it is a property of whatever operator new/delete you call.
I think DECL_IS_OPERATOR_{NEW,DELETE} is set on both of these, the standard ones have
also DECL_IS_REPLACEABLE_OPERATOR flag on them.
Anyway, handling this just in IRA doesn't seem to be enough, surely it should be also handled during alias analysis etc.
Comment 13 Jan Hubicka 2024-06-04 11:57:08 UTC
> Is the option supposed to be only about the standard global scope operator
> new/delete (_Znam etc.) or also user operator new/delete class methods?  If the
> former, then I agree it is a global property (or at least a per shared
> library/binary property, one can arrange stuff with symbol visibility etc.).
> Otherwise it is a property of whatever operator new/delete you call.
> I think DECL_IS_OPERATOR_{NEW,DELETE} is set on both of these, the standard
> ones have
> also DECL_IS_REPLACEABLE_OPERATOR flag on them.
> Anyway, handling this just in IRA doesn't seem to be enough, surely it should
> be also handled during alias analysis etc.

I can add the TBAA and IPA bits (actually have WIP patch for that
already).  But there is also __builtion_operator_new and
__builtin_operator_delete that tags sanity of new/delete with per-call
sensitivity (since calls originating from std library are more opaque
then direct calls done by users).  So we need per-call sensitivity
anyway which can be done by altenrative decl or flag in gimple_call.

If user is crazy enough to do fancy tricks in new/delete I think it may
be controlled by program state or so (so for some code new/delete may be
sane while for other code it does not need to be0

So having this working well with LTO is IMO reasonable thing to do from
QOI point of view...
Comment 14 user202729 2024-06-05 08:23:27 UTC
Regarding alias analysis. The current implementaion is such that:

compiler      | flag             | can alias?| can modify global?|
gcc           | sane             | no        | no                | << NEW
              | no-sane [default]| no        | yes               |
clang         | sane [default]   | no        | no                |
              | no-sane          | yes       | yes               |
[the standard]|                  | no        | yes               |

As pointed out in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110035#c13 , gcc already assume operator new's retuned pointer cannot alias any existing pointer. So no change is needed there.
Comment 15 Jan Hubicka 2024-06-05 13:00:25 UTC
> As pointed out in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110035#c13 , gcc
> already assume operator new's retuned pointer cannot alias any existing
> pointer. So no change is needed there.
Seems you are right, -fsane-operator-new is weaker than I assumed.
In addition to the assumption about modifying globals, one missed
optimization is removal of unused vectors (we have separate PR for that)

#include <vector>
void foo (int *);
int test()
{
        std::vector <int> a(1);
        return 0;
}
int test2(int size)
{
        int *a = new int;
        *a=0;
        delete a;
        return 0;
}
int test3(int size)
{
        int *a = (int *)::operator new (sizeof (int));
        *a=0;
        ::operator delete (a);
        return 0;
}
At -O2 Clang optimizes test and test2 "return 0", while we optimize only
test2.  libstdc++ uses clang's __builtin_operator_new that enables this
optimization based on claim that user's possibly insane new/delete can
not rely on internals of std::vector implementation and thus can not
reliably use memory being deleted.

I expected that -fassume-sane-operator-new should enable clang or GCC to
do the optimization for all three variants, but it does not (for clang).
I think it would be useful to have such a flag. There are number of
transformations we may want to do including
 - removal of dead stores to memory being deleted
 - removal of unnecesary reallocations i.e. when user creates empty
   vector and does sequence of push_backs.
 - avoiding new/delete for small vectors that fits to stack.
I believe those are sane for builtin_operator_new/delete. However since
clang does not define it this way, perhaps we will need
-fassume-really-sane-operator-new-delete for that?

It would be also nice to be able to determine functions that does paired
new/delete but no other side effects as pure/const etc.

BTW clang also does not optimize out load from global here:
int test3(int size)
{
        global = 0;
        int *a = (int *)::operator new (sizeof (int));
        *a=0;
        return global;
}
I would expect -fassume-sane-operator-new to make it possible?
Comment 16 user202729 2024-06-23 18:14:39 UTC
Hi,

I tried to implement patches that fix the two issues mentioned. Comments would be appreciated.

1. The operator new flag with many of the mentioned issues fixed: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654145.html

   Although I'm not entirely sure what to do with the link-time optimization part.

2. The optimization to move the definition to right before the use: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654700.html .

   As explained, I'm not sure if implementing it in the RTL phase; nevertheless, reviews and suggestions to improve the code, or explain how to implement it in GIMPLE phase would be appreciated.

------

Regarding the example code in the previous comment, it appears that the current analyzer is smart enough to do the optimization when the call to operator new is replaced by a pure function (curiously enough, it does not need to assume the returned value does not alias because both the values are 0). So, yes it should be possible (although I haven't test it with the proposed patch).