diff mbox series

[RFA] gimplify: avoid unnecessary copy of init array [PR105838]

Message ID 20221208220112.1700553-1-jason@redhat.com
State New
Headers show
Series [RFA] gimplify: avoid unnecessary copy of init array [PR105838] | expand

Commit Message

Jason Merrill Dec. 8, 2022, 10:01 p.m. UTC 
After the previous patches, I noticed that we were putting the array of
strings into .rodata, but then memcpying it into an automatic array, which
is pointless; we should be able to use it directly.

C++ doesn't allow us to do this for the backing array of an
initializer_list, but should be able to do it for the implementation detail
array we use to construct the backing array.

This doesn't happen automatically because TREE_ADDRESSABLE is set, and
gimplify_init_constructor uses that to decide whether to promote a variable
to static.  Ideally this could use escape analysis to recognize that the
address, though taken, never leaves the function; that should allow
promotion when we're only using the address for indexing within the
function, as in initlist-opt2.C.

But in initlist-opt1.C, we're passing the array address to another function,
so it definitely escapes; it's only safe in this case because it's calling a
standard library function that we know only uses it for indexing.  So, a
flag seems needed.  I first thought to put the flag on the TARGET_EXPR, but
the VAR_DECL seems more appropriate.

Bikeshedding, or other approaches, welcome.

	PR c++/105838

gcc/ChangeLog:

	* tree.h (DECL_NOT_OBSERVABLE): New.
	* tree-core.h (struct tree_decl_common): Mention it.
	* gimplify.cc (gimplify_init_constructor): Check it.

gcc/cp/ChangeLog:

	* call.cc (maybe_init_list_as_array): Set DECL_NOT_OBSERVABLE.

gcc/testsuite/ChangeLog:

	* g++.dg/tree-ssa/initlist-opt1.C: Check for static array.
	* g++.dg/tree-ssa/initlist-opt2.C: Likewise.
---
 gcc/tree-core.h                               | 3 ++-
 gcc/tree.h                                    | 5 +++++
 gcc/cp/call.cc                                | 4 +++-
 gcc/gimplify.cc                               | 3 ++-
 gcc/testsuite/g++.dg/tree-ssa/initlist-opt1.C | 1 +
 gcc/testsuite/g++.dg/tree-ssa/initlist-opt2.C | 1 +
 6 files changed, 14 insertions(+), 3 deletions(-)


base-commit: bd0485f20f4794f9787237706a6308473a8e9415

Comments

Jeff Law Dec. 17, 2022, 5:48 p.m. UTC | #1 
On 12/8/22 15:01, Jason Merrill via Gcc-patches wrote:
> After the previous patches, I noticed that we were putting the array of
> strings into .rodata, but then memcpying it into an automatic array, which
> is pointless; we should be able to use it directly.
> 
> C++ doesn't allow us to do this for the backing array of an
> initializer_list, but should be able to do it for the implementation detail
> array we use to construct the backing array.
> 
> This doesn't happen automatically because TREE_ADDRESSABLE is set, and
> gimplify_init_constructor uses that to decide whether to promote a variable
> to static.  Ideally this could use escape analysis to recognize that the
> address, though taken, never leaves the function; that should allow
> promotion when we're only using the address for indexing within the
> function, as in initlist-opt2.C.
> 
> But in initlist-opt1.C, we're passing the array address to another function,
> so it definitely escapes; it's only safe in this case because it's calling a
> standard library function that we know only uses it for indexing.  So, a
> flag seems needed.  I first thought to put the flag on the TARGET_EXPR, but
> the VAR_DECL seems more appropriate.
> 
> Bikeshedding, or other approaches, welcome.
> 
> 	PR c++/105838
> 
> gcc/ChangeLog:
> 
> 	* tree.h (DECL_NOT_OBSERVABLE): New.
> 	* tree-core.h (struct tree_decl_common): Mention it.
> 	* gimplify.cc (gimplify_init_constructor): Check it.
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (maybe_init_list_as_array): Set DECL_NOT_OBSERVABLE.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/tree-ssa/initlist-opt1.C: Check for static array.
> 	* g++.dg/tree-ssa/initlist-opt2.C: Likewise.
Presumably the DECL in question isn't actually compiler generated and 
thus DECL_ARTIFICIAL is not appropriate here.  Assuming that's the case, 
should we be setting DECL_NON_OBSERVABLE on VAR_DECLs with 
DECL_ARTIFICIAL set?  I don't think that needs to be done for this patch 
to move forward though.

There haven't been any objections, so OK.


Jeff
diff mbox series

Patch

diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index e146b133dbd..c0d63632c1e 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1808,7 +1808,8 @@  struct GTY(()) tree_decl_common {
      In VAR_DECL, PARM_DECL and RESULT_DECL, this is
      DECL_HAS_VALUE_EXPR_P.  */
   unsigned decl_flag_2 : 1;
-  /* In FIELD_DECL, this is DECL_PADDING_P.  */
+  /* In FIELD_DECL, this is DECL_PADDING_P.
+     In VAR_DECL, this is DECL_NOT_OBSERVABLE.  */
   unsigned decl_flag_3 : 1;
   /* Logically, these two would go in a theoretical base shared by var and
      parm decl. */
diff --git a/gcc/tree.h b/gcc/tree.h
index 23223ca0c87..7ba88fd16db 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3221,6 +3221,11 @@  extern void decl_fini_priority_insert (tree, priority_type);
 #define DECL_NONALIASED(NODE) \
   (VAR_DECL_CHECK (NODE)->base.nothrow_flag)
 
+/* In a VAR_DECL, nonzero if this variable is not observable by user code (and
+   so e.g. it can be promoted to static even if it's addressable).  */
+#define DECL_NOT_OBSERVABLE(NODE) \
+  (VAR_DECL_CHECK (NODE)->decl_common.decl_flag_3)
+
 /* This field is used to reference anything in decl.result and is meant only
    for use by the garbage collector.  */
 #define DECL_RESULT_FLD(NODE) \
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 14aa96dd328..a9052c64265 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -4247,7 +4247,9 @@  maybe_init_list_as_array (tree elttype, tree init)
 
   init_elttype = cp_build_qualified_type (init_elttype, TYPE_QUAL_CONST);
   tree arr = build_array_of_n_type (init_elttype, CONSTRUCTOR_NELTS (init));
-  return finish_compound_literal (arr, init, tf_none);
+  arr = finish_compound_literal (arr, init, tf_none);
+  DECL_NOT_OBSERVABLE (TARGET_EXPR_SLOT (arr)) = true;
+  return arr;
 }
 
 /* If we were going to call e.g. vector(initializer_list<string>) starting
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 250782b1140..87c913c48c5 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -5234,7 +5234,8 @@  gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	    && TREE_READONLY (object)
 	    && VAR_P (object)
 	    && !DECL_REGISTER (object)
-	    && (flag_merge_constants >= 2 || !TREE_ADDRESSABLE (object))
+	    && (flag_merge_constants >= 2 || !TREE_ADDRESSABLE (object)
+		|| DECL_NOT_OBSERVABLE (object))
 	    /* For ctors that have many repeated nonzero elements
 	       represented through RANGE_EXPRs, prefer initializing
 	       those through runtime loops over copies of large amounts
diff --git a/gcc/testsuite/g++.dg/tree-ssa/initlist-opt1.C b/gcc/testsuite/g++.dg/tree-ssa/initlist-opt1.C
index 053317b59d8..b1d2d25faf4 100644
--- a/gcc/testsuite/g++.dg/tree-ssa/initlist-opt1.C
+++ b/gcc/testsuite/g++.dg/tree-ssa/initlist-opt1.C
@@ -4,6 +4,7 @@ 
 
 // Test that we do range-initialization from const char *.
 // { dg-final { scan-tree-dump {_M_range_initialize<const char\* const\*>} "gimple" } }
+// { dg-final { scan-tree-dump {static const char.*72} "gimple" } }
 
 #include <string>
 #include <vector>
diff --git a/gcc/testsuite/g++.dg/tree-ssa/initlist-opt2.C b/gcc/testsuite/g++.dg/tree-ssa/initlist-opt2.C
index fc928bb5405..6730b758ac7 100644
--- a/gcc/testsuite/g++.dg/tree-ssa/initlist-opt2.C
+++ b/gcc/testsuite/g++.dg/tree-ssa/initlist-opt2.C
@@ -4,6 +4,7 @@ 
 
 // Check that we call the basic_string constructor once (and define it once).
 // { dg-final { scan-tree-dump-times {>::basic_string} 2 "gimple" } }
+// { dg-final { scan-tree-dump {static const char.*72} "gimple" } }
 
 #include <string>