[Bug c++/110137] New: implement clang -fassume-sane-operator-new

[Bug c++/110137] New: implement clang -fassume-sane-operator-new

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

            Bug ID: 110137
           Summary: implement clang -fassume-sane-operator-new
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
           Keywords|                            |missed-optimization
                 CC|                            |jwakely at redhat dot com

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
                 CC|jwakely at redhat dot com          |redi at gcc dot gnu.org
   Last reconfirmed|                            |2023-06-09
             Status|UNCONFIRMED                 |NEW

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This would be very nice, confirmed.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[hiraditya at msn dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #3 from AK <hiraditya at msn dot com> ---
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?

[Bug c++/110137] implement clang -fassume-sane-operator-new

[redi at gcc dot gnu.org]

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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[user202729 at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

user202729 <user202729 at protonmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |user202729 at protonmail dot com

--- Comment #6 from user202729 <user202729 at protonmail dot com> ---
Created attachment 58293
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58293&action=edit
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[user202729 at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #8 from user202729 <user202729 at protonmail dot com> ---
(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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #9 from Jan Hubicka <hubicka at ucw dot cz> ---
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...

[Bug c++/110137] implement clang -fassume-sane-operator-new

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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.

Re: [Bug c++/110137] implement clang -fassume-sane-operator-new

[Jan Hubicka]

> 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...

[Bug c++/110137] implement clang -fassume-sane-operator-new

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #13 from Jan Hubicka <hubicka at ucw dot cz> ---
> 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...

[Bug c++/110137] implement clang -fassume-sane-operator-new

[user202729 at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #14 from user202729 <user202729 at protonmail dot com> ---
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.

[Bug c++/110137] implement clang -fassume-sane-operator-new

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #15 from Jan Hubicka <hubicka at ucw dot cz> ---
> 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?

[Bug c++/110137] implement clang -fassume-sane-operator-new

[user202729 at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110137

--- Comment #16 from user202729 <user202729 at protonmail dot com> ---
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).