[PATCH] Implement -fassume-sane-operator-new [PR110137]

[PATCH] Implement -fassume-sane-operator-new [PR110137]

[user202729]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: a.patch --]
[-- Type: text/x-patch; name=a.patch, Size: 4584 bytes --]

From 14a8604907c89838577ff8560df9a3f9dc2d8afb Mon Sep 17 00:00:00 2001
From: user202729 <user202729@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.

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.

gcc/testsuite/ChangeLog:

	* g++.dg/sane-operator-new-1.C: New test.
	* g++.dg/sane-operator-new-2.C: New test.
	* g++.dg/sane-operator-new-3.C: New test.
---
 gcc/c-family/c.opt                         |  4 ++++
 gcc/ira.cc                                 | 23 ++++++++++++++++++++-
 gcc/testsuite/g++.dg/sane-operator-new-1.C | 12 +++++++++++
 gcc/testsuite/g++.dg/sane-operator-new-2.C | 12 +++++++++++
 gcc/testsuite/g++.dg/sane-operator-new-3.C | 24 ++++++++++++++++++++++
 5 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/sane-operator-new-1.C
 create mode 100644 gcc/testsuite/g++.dg/sane-operator-new-2.C
 create mode 100644 gcc/testsuite/g++.dg/sane-operator-new-3.C

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index fb34c3b7031..20c3ff77ee8 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1605,6 +1605,10 @@ fasm
 C ObjC C++ ObjC++ Var(flag_no_asm, 0)
 Recognize the \"asm\" keyword.
 
+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.
+
 ; Define extra predefined macros for use in libgcc.
 fbuilding-libgcc
 C ObjC C++ ObjC++ Undocumented Var(flag_building_libgcc)
diff --git a/gcc/ira.cc b/gcc/ira.cc
index 5642aea3caa..2902853a2bc 100644
--- a/gcc/ira.cc
+++ b/gcc/ira.cc
@@ -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)
+{
+  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)
+{
+  if (RTL_CONST_OR_PURE_CALL_P (insn))
+    return false;
+  if (flag_assume_sane_operator_new && is_call_operator_new_p (insn))
+    return false;
+  return true;
+}
+
 enum valid_equiv { valid_none, valid_combine, valid_reload };
 
 /* Verify that no store between START and the death of REG invalidates
@@ -3123,7 +3144,7 @@ validate_equiv_mem (rtx_insn *start, rtx reg, rtx memref)
 	     been changed and all hell breaks loose.  */
 	  ret = valid_combine;
 	  if (!MEM_READONLY_P (memref)
-	      && (!RTL_CONST_OR_PURE_CALL_P (insn)
+	      && (may_modify_memory_p (insn)
 		  || equiv_init_varies_p (XEXP (memref, 0))))
 	    return valid_none;
 	}
diff --git a/gcc/testsuite/g++.dg/sane-operator-new-1.C b/gcc/testsuite/g++.dg/sane-operator-new-1.C
new file mode 100644
index 00000000000..de81e1d92b9
--- /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" } */
+int a;
+float *b;
+int
+m ()
+{
+  int x = a;
+  b = new float;
+  return x + a;
+}
+/* { dg-final { scan-assembler-times {a\(%} 1 } } */
diff --git a/gcc/testsuite/g++.dg/sane-operator-new-2.C b/gcc/testsuite/g++.dg/sane-operator-new-2.C
new file mode 100644
index 00000000000..28fe880810e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/sane-operator-new-2.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2" } */
+int a;
+float *b;
+int
+m ()
+{
+  int x = a;
+  b = new float;
+  return x + a;
+}
+/* { dg-final { scan-assembler-times {a\(%} 2 } } */
diff --git a/gcc/testsuite/g++.dg/sane-operator-new-3.C b/gcc/testsuite/g++.dg/sane-operator-new-3.C
new file mode 100644
index 00000000000..17e9f0640e3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/sane-operator-new-3.C
@@ -0,0 +1,24 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+typedef __SIZE_TYPE__ size_t;
+extern "C" {
+  void* malloc (size_t);
+  void free (void*);
+  void abort (void);
+}
+int a = 5;
+float *b;
+void *
+__attribute__ ((noinline))
+operator new (size_t n) {
+  a = 70;
+  return malloc (n);
+}
+int
+main ()
+{
+  int x = a;
+  b = new float;
+  if (x + a != 75)
+    abort ();
+}
-- 
2.34.1

Re: [PATCH] Implement -fassume-sane-operator-new [PR110137]

[Jakub Jelinek]

On Wed, May 29, 2024 at 04:09:08AM +0000, user202729@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 <user202729@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

[PATCH v2] Implement -fassume-sane-operator-new [PR110137]

[user202729]

[-- Attachment #1: Type: text/plain, Size: 5082 bytes --]

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.

See the previous email for more context.

Issues raised has been fixed, except that it remains not very well integrated with LTO (but should not create wrong code either). As explained in the problem report, alias is not a concern.

While this is possible to do in a GIMPLE pass as well, I don't see a good way to do so because currently we don't have a pass that optimizes the redundant copy, and rely on IRA (which is a RTL pass) for that. It would be helpful if a better method is suggested.

Bootstrapped and regression tested on x86_64-pc-linux-gnu.

On Tuesday, June 4th, 2024 at 6:57 PM, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, May 29, 2024 at 04:09:08AM +0000, user202729@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 user202729@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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: a.patch --]
[-- Type: text/x-patch; name=a.patch, Size: 6063 bytes --]

From 687380e180a38b69681e505b6c0af38628906fdd Mon Sep 17 00:00:00 2001
From: user202729 <user202729@protonmail.com>
Date: Tue, 11 Jun 2024 12:17:40 +0800
Subject: [PATCH v2] Implement -fassume-sane-operator-new [PR110137]

	PR c++/110137

gcc/c-family/ChangeLog:

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

gcc/testsuite/ChangeLog:

	* g++.dg/sane-operator-new-1.C: New test.
	* g++.dg/sane-operator-new-2.C: New test.
	* g++.dg/sane-operator-new-3.C: New test.
---
 gcc/c-family/c.opt                            |  4 +++
 gcc/doc/invoke.texi                           | 15 +++++++++++
 gcc/ira.cc                                    | 27 ++++++++++++++++++-
 .../g++.target/i386/sane-operator-new-1.C     | 12 +++++++++
 .../g++.target/i386/sane-operator-new-2.C     | 12 +++++++++
 .../g++.target/i386/sane-operator-new-3.C     | 24 +++++++++++++++++
 6 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.target/i386/sane-operator-new-1.C
 create mode 100644 gcc/testsuite/g++.target/i386/sane-operator-new-2.C
 create mode 100644 gcc/testsuite/g++.target/i386/sane-operator-new-3.C

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index fb34c3b7031..20c3ff77ee8 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1605,6 +1605,10 @@ fasm
 C ObjC C++ ObjC++ Var(flag_no_asm, 0)
 Recognize the \"asm\" keyword.
 
+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.
+
 ; Define extra predefined macros for use in libgcc.
 fbuilding-libgcc
 C ObjC C++ ObjC++ Undocumented Var(flag_building_libgcc)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 2cba380718b..e562735f644 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -548,6 +548,7 @@ Objective-C and Objective-C++ Dialects}.
 -falign-jumps[=@var{n}[:@var{m}:[@var{n2}[:@var{m2}]]]]
 -falign-labels[=@var{n}[:@var{m}:[@var{n2}[:@var{m2}]]]]
 -falign-loops[=@var{n}[:@var{m}:[@var{n2}[:@var{m2}]]]]
+-fassume-sane-operator-new
 -fmin-function-alignment=[@var{n}]
 -fno-allocation-dce -fallow-store-data-races
 -fassociative-math  -fauto-profile  -fauto-profile[=@var{path}]
@@ -15271,6 +15272,20 @@ exactly determine which path is taken more often.
 
 Enabled by @option{-fprofile-use} and @option{-fauto-profile}.
 
+@opindex fassume-sane-operator-new
+@item -fassume-sane-operator-new
+Assumes that @code{operator new} does not modify global memory of interest to
+the compiler.  When combined with other optimization options, it may optimize
+the following code
+@smallexample
+int a, *b; // global
+
+a = 1;
+b = new int;
+return a;
+@end smallexample
+to always return 1.
+
 @opindex fprofile-values
 @item -fprofile-values
 If combined with @option{-fprofile-arcs}, it adds code so that some
diff --git a/gcc/ira.cc b/gcc/ira.cc
index 5642aea3caa..3eea6bd586d 100644
--- a/gcc/ira.cc
+++ b/gcc/ira.cc
@@ -3080,6 +3080,31 @@ validate_equiv_mem_from_store (rtx dest, const_rtx set ATTRIBUTE_UNUSED,
 
 static bool equiv_init_varies_p (rtx x);
 
+/* Checks if the instruction is a call to operator new. */
+static bool
+is_call_operator_new_p (rtx_insn *insn)
+{
+  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 that the
+   compiler cares about.
+   If false is returned, the compiler proved INSN never modify memory.  */
+static bool
+may_modify_memory_p (rtx_insn *insn)
+{
+  if (RTL_CONST_OR_PURE_CALL_P (insn))
+    return false;
+  if (flag_assume_sane_operator_new && is_call_operator_new_p (insn))
+    return false;
+  return true;
+}
+
 enum valid_equiv { valid_none, valid_combine, valid_reload };
 
 /* Verify that no store between START and the death of REG invalidates
@@ -3123,7 +3148,7 @@ validate_equiv_mem (rtx_insn *start, rtx reg, rtx memref)
 	     been changed and all hell breaks loose.  */
 	  ret = valid_combine;
 	  if (!MEM_READONLY_P (memref)
-	      && (!RTL_CONST_OR_PURE_CALL_P (insn)
+	      && (may_modify_memory_p (insn)
 		  || equiv_init_varies_p (XEXP (memref, 0))))
 	    return valid_none;
 	}
diff --git a/gcc/testsuite/g++.target/i386/sane-operator-new-1.C b/gcc/testsuite/g++.target/i386/sane-operator-new-1.C
new file mode 100644
index 00000000000..de81e1d92b9
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/sane-operator-new-1.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fassume-sane-operator-new" } */
+int a;
+float *b;
+int
+m ()
+{
+  int x = a;
+  b = new float;
+  return x + a;
+}
+/* { dg-final { scan-assembler-times {a\(%} 1 } } */
diff --git a/gcc/testsuite/g++.target/i386/sane-operator-new-2.C b/gcc/testsuite/g++.target/i386/sane-operator-new-2.C
new file mode 100644
index 00000000000..28fe880810e
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/sane-operator-new-2.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2" } */
+int a;
+float *b;
+int
+m ()
+{
+  int x = a;
+  b = new float;
+  return x + a;
+}
+/* { dg-final { scan-assembler-times {a\(%} 2 } } */
diff --git a/gcc/testsuite/g++.target/i386/sane-operator-new-3.C b/gcc/testsuite/g++.target/i386/sane-operator-new-3.C
new file mode 100644
index 00000000000..17e9f0640e3
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/sane-operator-new-3.C
@@ -0,0 +1,24 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+typedef __SIZE_TYPE__ size_t;
+extern "C" {
+  void* malloc (size_t);
+  void free (void*);
+  void abort (void);
+}
+int a = 5;
+float *b;
+void *
+__attribute__ ((noinline))
+operator new (size_t n) {
+  a = 70;
+  return malloc (n);
+}
+int
+main ()
+{
+  int x = a;
+  b = new float;
+  if (x + a != 75)
+    abort ();
+}
-- 
2.34.1