public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
@ 2016-03-10 17:04 Jakub Jelinek
  2016-03-10 17:19 ` Jason Merrill
  2016-03-10 17:35 ` Patrick Palka
  0 siblings, 2 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-10 17:04 UTC (permalink / raw)
  To: Jason Merrill; +Cc: gcc-patches

Hi!

As mentioned in the PR, the compile time and compile memory are wasted
if a large array is is using value or default initialization, and
if the resulting initializer value is simple enough, we can just share
it by all the elements.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-03-10  Patrick Palka  <ppalka@gcc.gnu.org>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/70001
	* constexpr.c (cxx_eval_vec_init_1): For pre_init case, reuse
	return value from cxx_eval_constant_expression from earlier
	elements if it is valid constant initializer requiring no
	relocations.

	* g++.dg/cpp0x/constexpr-70001-1.C: New test.
	* g++.dg/cpp0x/constexpr-70001-2.C: New test.
	* g++.dg/cpp0x/constexpr-70001-3.C: New test.

--- gcc/cp/constexpr.c.jj	2016-03-08 21:04:43.050564671 +0100
+++ gcc/cp/constexpr.c	2016-03-10 12:52:04.016852313 +0100
@@ -2340,6 +2340,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx
   vec<constructor_elt, va_gc> **p = &CONSTRUCTOR_ELTS (ctx->ctor);
   vec_alloc (*p, max + 1);
   bool pre_init = false;
+  tree pre_init_elt = NULL_TREE;
   unsigned HOST_WIDE_INT i;
 
   /* For the default constructor, build up a call to the default
@@ -2389,9 +2390,18 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	{
 	  /* Initializing an element using value or default initialization
 	     we just pre-built above.  */
-	  eltinit = (cxx_eval_constant_expression
-		     (&new_ctx, init,
-		      lval, non_constant_p, overflow_p));
+	  if (pre_init_elt == NULL_TREE)
+	    pre_init_elt
+	      = cxx_eval_constant_expression (&new_ctx, init, lval,
+					      non_constant_p, overflow_p);
+	  eltinit = pre_init_elt;
+	  /* Don't reuse the result of cxx_eval_constant_expression
+	     call if it isn't a constant initializer or if it requires
+	     relocations.  */
+	  if (initializer_constant_valid_p (pre_init_elt,
+					    TREE_TYPE (pre_init_elt))
+	      != null_pointer_node)
+	    pre_init_elt = NULL_TREE;
 	}
       else
 	{
--- gcc/testsuite/g++.dg/cpp0x/constexpr-70001-1.C.jj	2016-03-10 13:08:58.732932160 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-70001-1.C	2016-03-10 13:05:53.000000000 +0100
@@ -0,0 +1,13 @@
+// PR c++/70001
+// { dg-do compile { target c++11 } }
+
+struct B
+{
+  int a;
+  constexpr B () : a (0) { }
+};
+
+struct A
+{
+  B b[1 << 19];
+} c;
--- gcc/testsuite/g++.dg/cpp0x/constexpr-70001-2.C.jj	2016-03-10 13:09:01.866889167 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-70001-2.C	2016-03-10 13:07:27.000000000 +0100
@@ -0,0 +1,19 @@
+// PR c++/70001
+// { dg-do run { target c++11 } }
+
+struct B
+{
+  struct B *a;
+  constexpr B () : a (this) { }
+};
+
+constexpr int N = 1 << 4;
+struct A { B c[N]; } d;
+
+int
+main ()
+{
+  for (int i = 0; i < N; ++i)
+    if (d.c[i].a != &d.c[i])
+      __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/cpp0x/constexpr-70001-3.C.jj	2016-03-10 13:09:04.700850290 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-70001-3.C	2016-03-10 13:09:53.199184977 +0100
@@ -0,0 +1,26 @@
+// PR c++/70001
+// { dg-do compile { target c++11 } }
+
+#include <array>
+#include <complex>
+
+typedef std::complex<double> cd;
+
+const int LOG = 17;
+const int N = (1 << LOG);
+
+std::array<cd, N> a;
+std::array<cd, N> b;
+
+void
+foo (std::array<cd, N> &arr)
+{
+  std::array<std::array<cd, N>, LOG + 1> f;
+}
+
+int
+main ()
+{
+  foo (a);
+  foo (b);
+}

	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 17:04 [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001) Jakub Jelinek
@ 2016-03-10 17:19 ` Jason Merrill
  2016-03-10 17:35 ` Patrick Palka
  1 sibling, 0 replies; 12+ messages in thread
From: Jason Merrill @ 2016-03-10 17:19 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

OK.

Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 17:04 [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001) Jakub Jelinek
  2016-03-10 17:19 ` Jason Merrill
@ 2016-03-10 17:35 ` Patrick Palka
  2016-03-10 17:37   ` Jakub Jelinek
  1 sibling, 1 reply; 12+ messages in thread
From: Patrick Palka @ 2016-03-10 17:35 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jason Merrill, GCC Patches

On Thu, Mar 10, 2016 at 12:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> As mentioned in the PR, the compile time and compile memory are wasted
> if a large array is is using value or default initialization, and
> if the resulting initializer value is simple enough, we can just share
> it by all the elements.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-03-10  Patrick Palka  <ppalka@gcc.gnu.org>
>             Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/70001
>         * constexpr.c (cxx_eval_vec_init_1): For pre_init case, reuse
>         return value from cxx_eval_constant_expression from earlier
>         elements if it is valid constant initializer requiring no
>         relocations.
>
>         * g++.dg/cpp0x/constexpr-70001-1.C: New test.
>         * g++.dg/cpp0x/constexpr-70001-2.C: New test.
>         * g++.dg/cpp0x/constexpr-70001-3.C: New test.
>
> --- gcc/cp/constexpr.c.jj       2016-03-08 21:04:43.050564671 +0100
> +++ gcc/cp/constexpr.c  2016-03-10 12:52:04.016852313 +0100
> @@ -2340,6 +2340,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx
>    vec<constructor_elt, va_gc> **p = &CONSTRUCTOR_ELTS (ctx->ctor);
>    vec_alloc (*p, max + 1);
>    bool pre_init = false;
> +  tree pre_init_elt = NULL_TREE;
>    unsigned HOST_WIDE_INT i;
>
>    /* For the default constructor, build up a call to the default
> @@ -2389,9 +2390,18 @@ cxx_eval_vec_init_1 (const constexpr_ctx
>         {
>           /* Initializing an element using value or default initialization
>              we just pre-built above.  */
> -         eltinit = (cxx_eval_constant_expression
> -                    (&new_ctx, init,
> -                     lval, non_constant_p, overflow_p));
> +         if (pre_init_elt == NULL_TREE)
> +           pre_init_elt
> +             = cxx_eval_constant_expression (&new_ctx, init, lval,
> +                                             non_constant_p, overflow_p);
> +         eltinit = pre_init_elt;
> +         /* Don't reuse the result of cxx_eval_constant_expression
> +            call if it isn't a constant initializer or if it requires
> +            relocations.  */
> +         if (initializer_constant_valid_p (pre_init_elt,
> +                                           TREE_TYPE (pre_init_elt))
> +             != null_pointer_node)
> +           pre_init_elt = NULL_TREE;

Doesn't this mean that we call initializer_constant_valid_p at each
iteration?  This would slow down the non-constant case even further.
So I wonder if the return value of initializer_constant_valid_p could
be cached or something, since it seems like a potentially expensive
predicate.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 17:35 ` Patrick Palka
@ 2016-03-10 17:37   ` Jakub Jelinek
  2016-03-10 17:56     ` Jakub Jelinek
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-10 17:37 UTC (permalink / raw)
  To: Patrick Palka; +Cc: Jason Merrill, GCC Patches

On Thu, Mar 10, 2016 at 12:34:40PM -0500, Patrick Palka wrote:
> Doesn't this mean that we call initializer_constant_valid_p at each
> iteration?  This would slow down the non-constant case even further.
> So I wonder if the return value of initializer_constant_valid_p could
> be cached or something, since it seems like a potentially expensive
> predicate.

You're right, but I've already committed the patch.  I'll write an
incremental patch and test it.

	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 17:37   ` Jakub Jelinek
@ 2016-03-10 17:56     ` Jakub Jelinek
  2016-03-10 18:32       ` Patrick Palka
  2016-03-10 18:33       ` Jason Merrill
  0 siblings, 2 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-10 17:56 UTC (permalink / raw)
  To: Patrick Palka; +Cc: Jason Merrill, GCC Patches

On Thu, Mar 10, 2016 at 06:37:32PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 10, 2016 at 12:34:40PM -0500, Patrick Palka wrote:
> > Doesn't this mean that we call initializer_constant_valid_p at each
> > iteration?  This would slow down the non-constant case even further.
> > So I wonder if the return value of initializer_constant_valid_p could
> > be cached or something, since it seems like a potentially expensive
> > predicate.
> 
> You're right, but I've already committed the patch.  I'll write an
> incremental patch and test it.

So, like this?

2016-03-10  Jakub Jelinek  <jakub@redhat.com>

	PR c++/70001
	* constexpr.c (cxx_eval_vec_init_1): For pre_init, only
	call initializer_constant_valid_p on the first iteration.

--- gcc/cp/constexpr.c.jj	2016-03-10 12:52:04.000000000 +0100
+++ gcc/cp/constexpr.c	2016-03-10 18:45:13.416533853 +0100
@@ -2391,17 +2391,21 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	  /* Initializing an element using value or default initialization
 	     we just pre-built above.  */
 	  if (pre_init_elt == NULL_TREE)
-	    pre_init_elt
-	      = cxx_eval_constant_expression (&new_ctx, init, lval,
-					      non_constant_p, overflow_p);
-	  eltinit = pre_init_elt;
-	  /* Don't reuse the result of cxx_eval_constant_expression
-	     call if it isn't a constant initializer or if it requires
-	     relocations.  */
-	  if (initializer_constant_valid_p (pre_init_elt,
-					    TREE_TYPE (pre_init_elt))
-	      != null_pointer_node)
-	    pre_init_elt = NULL_TREE;
+	    {
+	      eltinit
+		= cxx_eval_constant_expression (&new_ctx, init, lval,
+						non_constant_p, overflow_p);
+	      /* Don't reuse the result of cxx_eval_constant_expression
+		 call if it isn't a constant initializer or if it requires
+		 relocations.  */
+	      if (i == 0
+		  && (initializer_constant_valid_p (eltinit,
+						    TREE_TYPE (eltinit))
+		      == null_pointer_node))
+		pre_init_elt = eltinit;
+	    }
+	  else
+	    eltinit = pre_init_elt;
 	}
       else
 	{


	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 17:56     ` Jakub Jelinek
@ 2016-03-10 18:32       ` Patrick Palka
  2016-03-10 18:40         ` Jakub Jelinek
  2016-03-10 18:33       ` Jason Merrill
  1 sibling, 1 reply; 12+ messages in thread
From: Patrick Palka @ 2016-03-10 18:32 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jason Merrill, GCC Patches

On Thu, Mar 10, 2016 at 12:56 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 10, 2016 at 06:37:32PM +0100, Jakub Jelinek wrote:
>> On Thu, Mar 10, 2016 at 12:34:40PM -0500, Patrick Palka wrote:
>> > Doesn't this mean that we call initializer_constant_valid_p at each
>> > iteration?  This would slow down the non-constant case even further.
>> > So I wonder if the return value of initializer_constant_valid_p could
>> > be cached or something, since it seems like a potentially expensive
>> > predicate.
>>
>> You're right, but I've already committed the patch.  I'll write an
>> incremental patch and test it.
>
> So, like this?
>
> 2016-03-10  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/70001
>         * constexpr.c (cxx_eval_vec_init_1): For pre_init, only
>         call initializer_constant_valid_p on the first iteration.
>
> --- gcc/cp/constexpr.c.jj       2016-03-10 12:52:04.000000000 +0100
> +++ gcc/cp/constexpr.c  2016-03-10 18:45:13.416533853 +0100
> @@ -2391,17 +2391,21 @@ cxx_eval_vec_init_1 (const constexpr_ctx
>           /* Initializing an element using value or default initialization
>              we just pre-built above.  */
>           if (pre_init_elt == NULL_TREE)
> -           pre_init_elt
> -             = cxx_eval_constant_expression (&new_ctx, init, lval,
> -                                             non_constant_p, overflow_p);
> -         eltinit = pre_init_elt;
> -         /* Don't reuse the result of cxx_eval_constant_expression
> -            call if it isn't a constant initializer or if it requires
> -            relocations.  */
> -         if (initializer_constant_valid_p (pre_init_elt,
> -                                           TREE_TYPE (pre_init_elt))
> -             != null_pointer_node)
> -           pre_init_elt = NULL_TREE;
> +           {
> +             eltinit
> +               = cxx_eval_constant_expression (&new_ctx, init, lval,
> +                                               non_constant_p, overflow_p);
> +             /* Don't reuse the result of cxx_eval_constant_expression
> +                call if it isn't a constant initializer or if it requires
> +                relocations.  */
> +             if (i == 0
> +                 && (initializer_constant_valid_p (eltinit,
> +                                                   TREE_TYPE (eltinit))
> +                     == null_pointer_node))
> +               pre_init_elt = eltinit;
> +           }
> +         else
> +           eltinit = pre_init_elt;
>         }
>        else
>         {

Looks fine to me :)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 17:56     ` Jakub Jelinek
  2016-03-10 18:32       ` Patrick Palka
@ 2016-03-10 18:33       ` Jason Merrill
  1 sibling, 0 replies; 12+ messages in thread
From: Jason Merrill @ 2016-03-10 18:33 UTC (permalink / raw)
  To: Jakub Jelinek, Patrick Palka; +Cc: GCC Patches

OK.

Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 18:32       ` Patrick Palka
@ 2016-03-10 18:40         ` Jakub Jelinek
  2016-03-10 20:33           ` Jakub Jelinek
  2016-03-11 14:27           ` Jason Merrill
  0 siblings, 2 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-10 18:40 UTC (permalink / raw)
  To: Patrick Palka; +Cc: Jason Merrill, GCC Patches

On Thu, Mar 10, 2016 at 01:32:10PM -0500, Patrick Palka wrote:
> Looks fine to me :)

On a closer look, this doesn't handle the multi-dimensional array cases,
and even for single-dimensional ones will not share the CONSTRUCTOR
if init_subob_ctx created one, and call init_subob_ctx many times
and in there in some cases e.g. build new new_ctx.object every time etc.

Is this also ok, if it passes bootstrap/regtest?

2016-03-10  Jakub Jelinek  <jakub@redhat.com>

	PR c++/70001
	* constexpr.c (cxx_eval_vec_init_1): Reuse CONSTRUCTOR initializers
	for 1..max even for multi-dimensional arrays, and reuse not just
	eltinit itself, but surrounding subobject CONSTRUCTOR too.

	* g++.dg/cpp0x/constexpr-70001-4.C: New test.

--- gcc/cp/constexpr.c.jj	2016-03-10 12:52:04.000000000 +0100
+++ gcc/cp/constexpr.c	2016-03-10 19:24:28.435537864 +0100
@@ -2340,7 +2340,6 @@ cxx_eval_vec_init_1 (const constexpr_ctx
   vec<constructor_elt, va_gc> **p = &CONSTRUCTOR_ELTS (ctx->ctor);
   vec_alloc (*p, max + 1);
   bool pre_init = false;
-  tree pre_init_elt = NULL_TREE;
   unsigned HOST_WIDE_INT i;
 
   /* For the default constructor, build up a call to the default
@@ -2370,6 +2369,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx
     {
       tree idx = build_int_cst (size_type_node, i);
       tree eltinit;
+      bool reuse = false;
       constexpr_ctx new_ctx;
       init_subob_ctx (ctx, new_ctx, idx, pre_init ? init : elttype);
       if (new_ctx.ctor != ctx->ctor)
@@ -2378,7 +2378,10 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	{
 	  /* A multidimensional array; recurse.  */
 	  if (value_init || init == NULL_TREE)
-	    eltinit = NULL_TREE;
+	    {
+	      eltinit = NULL_TREE;
+	      reuse = i == 0;
+	    }
 	  else
 	    eltinit = cp_build_array_ref (input_location, init, idx,
 					  tf_warning_or_error);
@@ -2390,18 +2393,9 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	{
 	  /* Initializing an element using value or default initialization
 	     we just pre-built above.  */
-	  if (pre_init_elt == NULL_TREE)
-	    pre_init_elt
-	      = cxx_eval_constant_expression (&new_ctx, init, lval,
-					      non_constant_p, overflow_p);
-	  eltinit = pre_init_elt;
-	  /* Don't reuse the result of cxx_eval_constant_expression
-	     call if it isn't a constant initializer or if it requires
-	     relocations.  */
-	  if (initializer_constant_valid_p (pre_init_elt,
-					    TREE_TYPE (pre_init_elt))
-	      != null_pointer_node)
-	    pre_init_elt = NULL_TREE;
+	  eltinit = cxx_eval_constant_expression (&new_ctx, init, lval,
+						  non_constant_p, overflow_p);
+	  reuse = i == 0;
 	}
       else
 	{
@@ -2427,6 +2421,23 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	}
       else
 	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
+      /* Don't reuse the result of cxx_eval_constant_expression
+	 call if it isn't a constant initializer or if it requires
+	 relocations.  */
+      if (reuse
+	  && max > 1
+	  && (initializer_constant_valid_p (eltinit, TREE_TYPE (eltinit))
+	      == null_pointer_node))
+	{
+	  if (new_ctx.ctor != ctx->ctor)
+	    eltinit = new_ctx.ctor;
+	  for (i = 1; i < max; ++i)
+	    {
+	      idx = build_int_cst (size_type_node, i);
+	      CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
+	    }
+	  break;
+	}
     }
 
   if (!*non_constant_p)
--- gcc/testsuite/g++.dg/cpp0x/constexpr-70001-4.C.jj	2016-03-10 19:28:13.386481311 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-70001-4.C	2016-03-10 19:28:43.295074924 +0100
@@ -0,0 +1,13 @@
+// PR c++/70001
+// { dg-do compile { target c++11 } }
+
+struct B
+{
+  int a;
+  constexpr B () : a (0) { }
+};
+
+struct A
+{
+  B b[1 << 19][1][1][1];
+} c;


	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 18:40         ` Jakub Jelinek
@ 2016-03-10 20:33           ` Jakub Jelinek
  2016-03-11 14:27           ` Jason Merrill
  1 sibling, 0 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-10 20:33 UTC (permalink / raw)
  To: Patrick Palka, Jason Merrill; +Cc: GCC Patches

On Thu, Mar 10, 2016 at 07:39:57PM +0100, Jakub Jelinek wrote:
> Is this also ok, if it passes bootstrap/regtest?
> 
> 2016-03-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/70001
> 	* constexpr.c (cxx_eval_vec_init_1): Reuse CONSTRUCTOR initializers
> 	for 1..max even for multi-dimensional arrays, and reuse not just
> 	eltinit itself, but surrounding subobject CONSTRUCTOR too.
> 
> 	* g++.dg/cpp0x/constexpr-70001-4.C: New test.

Successfully bootstrapped/regtested on x86_64-linux and i686-linux.

	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-10 18:40         ` Jakub Jelinek
  2016-03-10 20:33           ` Jakub Jelinek
@ 2016-03-11 14:27           ` Jason Merrill
  2016-03-11 14:50             ` Jakub Jelinek
  2016-03-11 19:38             ` Jakub Jelinek
  1 sibling, 2 replies; 12+ messages in thread
From: Jason Merrill @ 2016-03-11 14:27 UTC (permalink / raw)
  To: Jakub Jelinek, Patrick Palka; +Cc: GCC Patches

On 03/10/2016 01:39 PM, Jakub Jelinek wrote:
> +      /* Don't reuse the result of cxx_eval_constant_expression
> +	 call if it isn't a constant initializer or if it requires
> +	 relocations.  */

Let's phrase this positively ("Reuse the result if...").

> +	  if (new_ctx.ctor != ctx->ctor)
> +	    eltinit = new_ctx.ctor;
> +	  for (i = 1; i < max; ++i)
> +	    {
> +	      idx = build_int_cst (size_type_node, i);
> +	      CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
> +	    }

This is going to use the same CONSTRUCTOR for all the elements, which 
will lead to problems if we then store into a subobject of one of the 
elements and see that reflected in all the others as well.  We need to 
unshare_expr when reusing the initializer.

Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-11 14:27           ` Jason Merrill
@ 2016-03-11 14:50             ` Jakub Jelinek
  2016-03-11 19:38             ` Jakub Jelinek
  1 sibling, 0 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-11 14:50 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Patrick Palka, GCC Patches

On Fri, Mar 11, 2016 at 09:27:54AM -0500, Jason Merrill wrote:
> On 03/10/2016 01:39 PM, Jakub Jelinek wrote:
> >+      /* Don't reuse the result of cxx_eval_constant_expression
> >+	 call if it isn't a constant initializer or if it requires
> >+	 relocations.  */
> 
> Let's phrase this positively ("Reuse the result if...").
> 
> >+	  if (new_ctx.ctor != ctx->ctor)
> >+	    eltinit = new_ctx.ctor;
> >+	  for (i = 1; i < max; ++i)
> >+	    {
> >+	      idx = build_int_cst (size_type_node, i);
> >+	      CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
> >+	    }
> 
> This is going to use the same CONSTRUCTOR for all the elements, which will
> lead to problems if we then store into a subobject of one of the elements
> and see that reflected in all the others as well.  We need to unshare_expr
> when reusing the initializer.

Well, but then even what has been already committed is unsafe,
initializer_constant_valid_p can return null_pointer_node even on
CONSTRUCTOR, or CONSTRUCTOR holding CONSTRUCTORs etc.
So, either we need to unshare_expr it in every case, so
CONSTRUCTOR_APPEND_ELT (*p, idx, unshare_expr (eltinit));
or alternatively we could use some flag on CONSTRUCTOR to mark (possibly)
shared ctors and unshare them upon constexpr store to them, or
unshare whenever we store.

What would be a testcase for the unsharing?
Following still works:

// PR c++/70001
// { dg-do compile { target c++14 } }

struct B
{
  int a;
  constexpr B () : a (0) { }
  constexpr B (int x) : a (x) { }
};
struct C
{
  B c;
  constexpr C () : c (0) { }
};
struct A
{
  B b[1 << 4];
};
struct D
{
  C d[1 << 4];
};

constexpr int
foo (int a, int b)
{
  A c;
  c.b[a].a += b;
  c.b[b].a += a;
  return c.b[0].a + c.b[a].a + c.b[b].a;
}

constexpr int d = foo (1, 2);
constexpr int e = foo (0, 3);
constexpr int f = foo (2, 4);
static_assert (d == 3 && e == 6 && f == 6, "");

constexpr int
bar (int a, int b)
{
  D c;
  c.d[a].c.a += b;
  c.d[b].c.a += a;
  return c.d[0].c.a + c.d[a].c.a + c.d[b].c.a;
}

constexpr int g = bar (1, 2);
constexpr int h = bar (0, 3);
constexpr int i = bar (2, 4);
static_assert (g == 3 && h == 6 && i == 6, "");

	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)
  2016-03-11 14:27           ` Jason Merrill
  2016-03-11 14:50             ` Jakub Jelinek
@ 2016-03-11 19:38             ` Jakub Jelinek
  1 sibling, 0 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-03-11 19:38 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Patrick Palka, GCC Patches

On Fri, Mar 11, 2016 at 09:27:54AM -0500, Jason Merrill wrote:
> On 03/10/2016 01:39 PM, Jakub Jelinek wrote:
> >+      /* Don't reuse the result of cxx_eval_constant_expression
> >+	 call if it isn't a constant initializer or if it requires
> >+	 relocations.  */
> 
> Let's phrase this positively ("Reuse the result if...").
> 
> >+	  if (new_ctx.ctor != ctx->ctor)
> >+	    eltinit = new_ctx.ctor;
> >+	  for (i = 1; i < max; ++i)
> >+	    {
> >+	      idx = build_int_cst (size_type_node, i);
> >+	      CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
> >+	    }
> 
> This is going to use the same CONSTRUCTOR for all the elements, which will
> lead to problems if we then store into a subobject of one of the elements
> and see that reflected in all the others as well.  We need to unshare_expr
> when reusing the initializer.

Ok, here is a new version with unshare_expr and reworded comment.
I haven't been successful with writing a testcase where the unshare_expr
would matter, but have included what I've tried in the patch anyway.
Compile-time wise the unshare_expr is not very costly, and e.g. for the
other testcase in this patch gives a nice compile time speedup.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-03-11  Jakub Jelinek  <jakub@redhat.com>

	PR c++/70001
	* constexpr.c (cxx_eval_vec_init_1): Reuse CONSTRUCTOR initializers
	for 1..max even for multi-dimensional arrays.  Call unshare_expr
	on it.

	* g++.dg/cpp0x/constexpr-70001-4.C: New test.
	* g++.dg/cpp1y/pr70001.C: New test.

--- gcc/cp/constexpr.c.jj	2016-03-10 12:52:04.000000000 +0100
+++ gcc/cp/constexpr.c	2016-03-11 19:24:28.435537864 +0100
@@ -2340,7 +2340,6 @@ cxx_eval_vec_init_1 (const constexpr_ctx
   vec<constructor_elt, va_gc> **p = &CONSTRUCTOR_ELTS (ctx->ctor);
   vec_alloc (*p, max + 1);
   bool pre_init = false;
-  tree pre_init_elt = NULL_TREE;
   unsigned HOST_WIDE_INT i;
 
   /* For the default constructor, build up a call to the default
@@ -2370,6 +2369,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx
     {
       tree idx = build_int_cst (size_type_node, i);
       tree eltinit;
+      bool reuse = false;
       constexpr_ctx new_ctx;
       init_subob_ctx (ctx, new_ctx, idx, pre_init ? init : elttype);
       if (new_ctx.ctor != ctx->ctor)
@@ -2378,7 +2378,10 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	{
 	  /* A multidimensional array; recurse.  */
 	  if (value_init || init == NULL_TREE)
-	    eltinit = NULL_TREE;
+	    {
+	      eltinit = NULL_TREE;
+	      reuse = i == 0;
+	    }
 	  else
 	    eltinit = cp_build_array_ref (input_location, init, idx,
 					  tf_warning_or_error);
@@ -2390,18 +2393,9 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	{
 	  /* Initializing an element using value or default initialization
 	     we just pre-built above.  */
-	  if (pre_init_elt == NULL_TREE)
-	    pre_init_elt
-	      = cxx_eval_constant_expression (&new_ctx, init, lval,
-					      non_constant_p, overflow_p);
-	  eltinit = pre_init_elt;
-	  /* Don't reuse the result of cxx_eval_constant_expression
-	     call if it isn't a constant initializer or if it requires
-	     relocations.  */
-	  if (initializer_constant_valid_p (pre_init_elt,
-					    TREE_TYPE (pre_init_elt))
-	      != null_pointer_node)
-	    pre_init_elt = NULL_TREE;
+	  eltinit = cxx_eval_constant_expression (&new_ctx, init, lval,
+						  non_constant_p, overflow_p);
+	  reuse = i == 0;
 	}
       else
 	{
@@ -2427,6 +2421,23 @@ cxx_eval_vec_init_1 (const constexpr_ctx
 	}
       else
 	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
+      /* Reuse the result of cxx_eval_constant_expression call
+	  from the first iteration to all others if it is a constant
+	  initializer that doesn't require relocations.  */
+      if (reuse
+	  && max > 1
+	  && (initializer_constant_valid_p (eltinit, TREE_TYPE (eltinit))
+	      == null_pointer_node))
+	{
+	  if (new_ctx.ctor != ctx->ctor)
+	    eltinit = new_ctx.ctor;
+	  for (i = 1; i < max; ++i)
+	    {
+	      idx = build_int_cst (size_type_node, i);
+	      CONSTRUCTOR_APPEND_ELT (*p, idx, unshare_expr (eltinit));
+	    }
+	  break;
+	}
     }
 
   if (!*non_constant_p)
--- gcc/testsuite/g++.dg/cpp0x/constexpr-70001-4.C.jj	2016-03-10 19:28:13.386481311 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-70001-4.C	2016-03-10 19:28:43.295074924 +0100
@@ -0,0 +1,13 @@
+// PR c++/70001
+// { dg-do compile { target c++11 } }
+
+struct B
+{
+  int a;
+  constexpr B () : a (0) { }
+};
+
+struct A
+{
+  B b[1 << 19][1][1][1];
+} c;
--- gcc/testsuite/g++.dg/cpp1y/pr70001.C.jj	2016-03-11 18:22:15.526046513 +0100
+++ gcc/testsuite/g++.dg/cpp1y/pr70001.C	2016-03-11 18:21:43.000000000 +0100
@@ -0,0 +1,49 @@
+// PR c++/70001
+// { dg-do compile { target c++14 } }
+
+struct B
+{
+  int a;
+  constexpr B () : a (0) { }
+  constexpr B (int x) : a (x) { }
+};
+struct C
+{
+  B c;
+  constexpr C () : c (0) { }
+};
+struct A
+{
+  B b[1 << 4];
+};
+struct D
+{
+  C d[1 << 4];
+};
+
+constexpr int
+foo (int a, int b)
+{
+  A c;
+  c.b[a].a += b;
+  c.b[b].a += a;
+  return c.b[0].a + c.b[a].a + c.b[b].a;
+}
+
+constexpr int
+bar (int a, int b)
+{
+  D c;
+  c.d[a].c.a += b;
+  c.d[b].c.a += a;
+  return c.d[0].c.a + c.d[a].c.a + c.d[b].c.a;
+}
+
+constexpr int d = foo (1, 2);
+constexpr int e = foo (0, 3);
+constexpr int f = foo (2, 4);
+constexpr int g = bar (1, 2);
+constexpr int h = bar (0, 3);
+constexpr int i = bar (2, 4);
+static_assert (d == 3 && e == 6 && f == 6, "");
+static_assert (g == 3 && h == 6 && i == 6, "");


	Jakub

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-03-11 19:38 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-10 17:04 [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001) Jakub Jelinek
2016-03-10 17:19 ` Jason Merrill
2016-03-10 17:35 ` Patrick Palka
2016-03-10 17:37   ` Jakub Jelinek
2016-03-10 17:56     ` Jakub Jelinek
2016-03-10 18:32       ` Patrick Palka
2016-03-10 18:40         ` Jakub Jelinek
2016-03-10 20:33           ` Jakub Jelinek
2016-03-11 14:27           ` Jason Merrill
2016-03-11 14:50             ` Jakub Jelinek
2016-03-11 19:38             ` Jakub Jelinek
2016-03-10 18:33       ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).