Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jakub Jelinek]

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  

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.0 +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 **p = _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 (_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 (_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;
--- 

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jakub Jelinek]

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

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jason Merrill]

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

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jakub Jelinek]

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

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jakub Jelinek]

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  

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.0 +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 **p = _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 (_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 (_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

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jason Merrill]

OK.

Jason

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Patrick Palka]

On Thu, Mar 10, 2016 at 12:56 PM, Jakub Jelinek  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  
>
> 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.0 +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 (_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 (_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 :)

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jakub Jelinek]

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  

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.0 +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 (_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 (_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

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jakub Jelinek]

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

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Patrick Palka]

On Thu, Mar 10, 2016 at 12:03 PM, Jakub Jelinek  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  
> Jakub Jelinek  
>
> 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 **p = _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
> -(_ctx, init,
> - lval, non_constant_p, overflow_p));
> + if (pre_init_elt == NULL_TREE)
> +   pre_init_elt
> + = cxx_eval_constant_expression (_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.

Re: [C++ PATCH] Reuse certain cxx_eval_constant_expression results in cxx_eval_vec_init_1 (PR c++/70001)

[Jason Merrill]

OK.

Jason