On Wed, May 29, 2024 at 04:09:08AM +0000, user202...@protonmail.com wrote:
> This patch implements the flag -fassume-sane-operator-new as suggested in
> PR110137. When the flag is enabled, it is assumed that operator new does not
> modify global memory.
>
> While this patch is not powerful enough to handle the original issue in
> PR110035, it allows the optimizer to handle some simpler case (e.g. load from
> global memory with fixed address), as demonstrated in the test
> sane-operator-new-1.C.
>
> To handle the original issue in PR110035, some other improvement to the
> optimizer is needed, which will be sent as subsequent patches.
>
> Bootstrapped and regression tested on x86_64-pc-linux-gnu.
> From 14a8604907c89838577ff8560df9a3f9dc2d8afb Mon Sep 17 00:00:00 2001
> From: user202729 <user202...@protonmail.com>
> Date: Fri, 24 May 2024 17:40:55 +0800
> Subject: [PATCH] Implement -fassume-sane-operator-new [PR110137]
>
> PR c++/110137
>
> gcc/c-family/ChangeLog:
>
> * c.opt: New option.
You need c.opt (fassume-sane-operator-new): New option.
> gcc/ChangeLog:
>
> * ira.cc (is_call_operator_new_p): New function.
> (may_modify_memory_p): Likewise.
> (validate_equiv_mem): Modify to use may_modify_memory_p.
The patch doesn't update doc/invoke.texi with the description of
what the option does, that is essential.
> +fassume-sane-operator-new
> +C++ Optimization Var(flag_assume_sane_operator_new)
> +Assume operator new does not have any side effect other than the allocation.
Is it just about operator new and not about operator delete as well in
clang?
Is it about all operator new or just the replaceable ones (standard ones in
global scope, those also have DECL_IS_REPLACEABLE_OPERATOR flag on them).
Depending on this, if the flag is about only replaceable ones, I think it is
a global property, so for LTO it should be merged as if there is a single TU
which uses this flag, it is set for the whole LTO compilation (or should it
be only for TUs with that flag which actually use such operator new calls?).
If it is all operators new, then it is a local property in each function (or
even better a property of the operators actually) and we should track
somewhere in cfun whether a function compiled with that flag calls operator
new and whether a function compiled without that flag calls operator new.
Then e.g. during inlining merge it, such that if both the functions invoke
operator new and they disagree on whether it is sane or not, the non-sane
case wins.
> --- a/gcc/ira.cc
> +++ b/gcc/ira.cc
This surely is much more important to handle in the alias oracle, not just
IRA.
> @@ -3080,6 +3080,27 @@ validate_equiv_mem_from_store (rtx dest, const_rtx set
> ATTRIBUTE_UNUSED,
>
> static bool equiv_init_varies_p (rtx x);
>
> +static bool is_call_operator_new_p (rtx_insn *insn)
Formatting, static bool on one line, is_call_... on another one.
And needs a function comment.
> +{
> + if (!CALL_P (insn))
> + return false;
> + tree fn = get_call_fndecl (insn);
> + if (fn == NULL_TREE)
> + return false;
> + return DECL_IS_OPERATOR_NEW_P (fn);
> +}
> +
> +/* Returns true if there is a possibility that INSN may modify memory.
> + If false is returned, the compiler proved INSN never modify memory. */
> +static bool may_modify_memory_p (rtx_insn *insn)
Again, missing newline instead of space after bool.
Not sure about the name of this function, even sane replaceable operator new
may modify memory (it actually has to), just shouldn't modify memory
the compiler cares about.
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/sane-operator-new-1.C
> @@ -0,0 +1,12 @@
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -fassume-sane-operator-new" } */
If the tests are x86 specific, they should go to g++.target/i386/ directory.
But as I said earlier, it would be better to handle optimizations like that
on GIMPLE too and then you can test that say on optimized dump on all
targets.
Jakub
