[C++PATCH,PR87322] move cp_evaluated up to tsubst all lambda parms

Message ID ord0o4jc2r.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [C++PATCH,PR87322] move cp_evaluated up to tsubst all lambda parms
Related show

Commit Message

Alexandre Oliva Feb. 7, 2019, 7:24 a.m.
From: Alexandre Oliva <aoliva@redhat.com>


A lambda capture variable initialized with a lambda expr taking more
than one parameter got us confused.

The first problem was that the parameter list was cut short during
tsubsting because we tsubsted it with cp_unevaluated_operand.  We
reset it right after, to tsubst the function body, so I've moved the
reset up so that it's in effect while processing the parameters as
well.

The second problem was that the lambda expr appeared twice, once in a
decltype that gave the capture variable its type, and once in its
initializer.  This caused us to instantiate two separate lambda exprs
and closure types, and then to flag that the lambda expr in the
initializer could not be converted to the unrelated closure type
determined for the capture variable.  Recording the tsubsted expr in
the local specialization map, and retrieving it for reuse fixed it.

Regstrapped on x86_64- and i686-linux-gnu.  Ok to install?


for  gcc/cp/ChangeLog

	PR c++/87322
	* pt.c (tsubst_lambda_expr): Avoid duplicate tsubsting.
	Move cp_evaluated resetting before signature tsubsting.

for  gcc/testsuite/ChangeLog

	PR c++/87322
	* g++.dg/cpp1y/pr87322.C: New.
---
 gcc/cp/pt.c                          |   18 ++++++++++++++----
 gcc/testsuite/g++.dg/cpp1y/pr87322.C |   23 +++++++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr87322.C



-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe

Comments

Jason Merrill Feb. 7, 2019, 4:58 p.m. | #1
On 2/7/19 2:24 AM, Alexandre Oliva wrote:
> 

> From: Alexandre Oliva <aoliva@redhat.com>

> 

> A lambda capture variable initialized with a lambda expr taking more

> than one parameter got us confused.

> 

> The first problem was that the parameter list was cut short during

> tsubsting because we tsubsted it with cp_unevaluated_operand.  We

> reset it right after, to tsubst the function body, so I've moved the

> reset up so that it's in effect while processing the parameters as

> well.

> 

> The second problem was that the lambda expr appeared twice, once in a

> decltype that gave the capture variable its type, and once in its

> initializer.  This caused us to instantiate two separate lambda exprs

> and closure types, and then to flag that the lambda expr in the

> initializer could not be converted to the unrelated closure type

> determined for the capture variable.  Recording the tsubsted expr in

> the local specialization map, and retrieving it for reuse fixed it.

> 

> Regstrapped on x86_64- and i686-linux-gnu.  Ok to install?

> 

> 

> for  gcc/cp/ChangeLog

> 

> 	PR c++/87322

> 	* pt.c (tsubst_lambda_expr): Avoid duplicate tsubsting.

> 	Move cp_evaluated resetting before signature tsubsting.

> 

> for  gcc/testsuite/ChangeLog

> 

> 	PR c++/87322

> 	* g++.dg/cpp1y/pr87322.C: New.

> ---

>   gcc/cp/pt.c                          |   18 ++++++++++++++----

>   gcc/testsuite/g++.dg/cpp1y/pr87322.C |   23 +++++++++++++++++++++++

>   2 files changed, 37 insertions(+), 4 deletions(-)

>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr87322.C

> 

> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c

> index b8fbf4046f07..3dffa8d2de88 100644

> --- a/gcc/cp/pt.c

> +++ b/gcc/cp/pt.c

> @@ -17912,8 +17912,17 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)

>     tree oldfn = lambda_function (t);

>     in_decl = oldfn;

>   

> +  /* If we have already specialized this lambda expr, reuse it.  See

> +     PR c++/86322.  */


Wrong PR number.

> +  if (local_specializations)

> +    if (tree r = retrieve_local_specialization (t))

> +      return r;


Hmm, I would expect this to do the wrong thing for pack expansion of a 
lambda, giving us only one rather than one per element of the expansion. 
  For instance, in lambda-variadic5.C we should get three instantiations 
of the "print" function template; can you add that check to the test?

The cp_evaluated change is OK.

Jason
Alexandre Oliva Feb. 8, 2019, 6:58 a.m. | #2
On Feb  7, 2019, Jason Merrill <jason@redhat.com> wrote:

>> +     PR c++/86322.  */


> Wrong PR number.


Thanks

>> +  if (local_specializations)

>> +    if (tree r = retrieve_local_specialization (t))

>> +      return r;


> Hmm, I would expect this to do the wrong thing for pack expansion of a

> lambda, giving us only one rather than one per element of the

> expansion.


Oooh.  Indeed, I hadn't realized this was possible.

I think we should separate local_specializations arising from different
pack expansion bindings, considering they won't always appear in tsubst
args:


diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 0177d7f77891..9f954698f454 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -11698,6 +11698,10 @@ gen_elem_of_pack_expansion_instantiation (tree pattern,
       ARGUMENT_PACK_SELECT_INDEX (aps) = index;
     }
 
+  // Any local specialization bindings arising from this substitution
+  // cannot be reused for a different INDEX.
+  local_specialization_stack lss (lss_copy);
+
   /* Substitute into the PATTERN with the (possibly altered)
      arguments.  */
   if (pattern == in_decl)


> For instance, in lambda-variadic5.C we should get three

> instantiations of the "print" function template; can you add that

> check to the test?


This causes lambda-variadic5.C to get 6 separate lambdas, instead of 2,
so it passes after the following patchlet:

diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C
index 97f64cd761a7..1f931757b72f 100644
--- a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C
@@ -1,5 +1,7 @@
 // PR c++/47226
 // { dg-do compile { target c++11 } }
+// { dg-options "-fdump-tree-original" }
+// { dg-final { scan-tree-dump-times "::<lambda\\(\\)> \\(null\\)" 6 "original" } }
 
 template<class T>
 void print(const T&) {}


Now, should I adjust lambda-variadic5.C to check for the presence of the
expencted count of lambdas in compiler dumps?  Or should I go for a new
test?

Thinking of how to test for it drew my attention to issues about lambda
naming, that may have implications on ABI and duplicate code removal.
There are two issues I'm concerned with:

- we increment lambda_cnt when introducing a lambda within a generic,
  and then increment it again as we specialize, partially or fully, each
  lambda expr/object/type set.  This makes for gaps in numbering of full
  specializations and, being a global counter, does not ensure the
  assignment of the same symbol name to corresponding instantiations in
  separate translation units

- we don't seem to attempt to reuse lambdas across different
  specializations of argument pack expansions.  I guess that's ok, we
  don't attempt to reuse them across different specializations of the
  enclosing template function or class either, but, unlike enclosing
  contexts, different argument pack indices do not mandate different
  mangled names.  This came up because I was thinking of having a
  sub-map in local_specializations, so that the lambda would map to
  another map in which we could look up template arguments to find a
  specific instance.  That wouldn't work, in part because args does not
  provide the entire set of substitutions, and in another part because
  we don't seem to be able to identify, in patterns undergoing
  substitution but before actually performing the substitution, the set
  of (implied?) template arguments that might affect the substitution
  result.  To make this concrete: in

    [&t]{return([]{return(0);}()+t);}...

  we might be able to reuse the nested lambda across expansions of the
  enclosing lambda.  except that, being nested, their contexts wouldn't
  allow that.  but what if they were siblings, say:

    ([&t]{return(t);}()+[]{return(0);}())...
  
  here the latter is in no way affected by the t pack, so we could
  create a single lambda, reusing it for all indices in t.  The patchlet
  above will stand in the way of it, but IIUC it doesn't matter, we're
  not supposed/expected to do that, even when the decision on whether or
  not to reuse might affect symbol naming, e.g. if it appears in an
  initializer of a data member.


Here's the patch I'm testing.  OK if it passes?


[PR87322] move cp_evaluated up to tsubst all lambda parms

From: Alexandre Oliva <aoliva@redhat.com>


A lambda capture variable initialized with a lambda expr taking more
than one parameter got us confused.

The first problem was that the parameter list was cut short during
tsubsting because we tsubsted it with cp_unevaluated_operand.  We
reset it right after, to tsubst the function body, so I've moved the
reset up so that it's in effect while processing the parameters as
well.

The second problem was that the lambda expr appeared twice, once in a
decltype that gave the capture variable its type, and once in its
initializer.  This caused us to instantiate two separate lambda exprs
and closure types, and then to flag that the lambda expr in the
initializer could not be converted to the unrelated closure type
determined for the capture variable.  Recording the tsubsted expr in
the local specialization map, and retrieving it for reuse fixed it.
However, that required some care to avoid reusing the lambda expr
across different indices in pack expansions.


for  gcc/cp/ChangeLog

	PR c++/87322
	* pt.c (tsubst_lambda_expr): Avoid duplicate tsubsting.
	Move cp_evaluated resetting before signature tsubsting.
	(gen_elem_of_pack_expansion_instantiation): Separate local
	specializations per index.

for  gcc/testsuite/ChangeLog

	PR c++/87322
	* g++.dg/cpp1y/pr87322.C: New.
	* g++.dg/cpp0x/lambda/lambda-variadic5.C: Test that we
	instantiate the expected number of lambda functions.
---
 gcc/cp/pt.c                                        |   22 ++++++++++++++++---
 .../g++.dg/cpp0x/lambda/lambda-variadic5.C         |    2 ++
 gcc/testsuite/g++.dg/cpp1y/pr87322.C               |   23 ++++++++++++++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr87322.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index b8fbf4046f07..9f954698f454 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -11698,6 +11698,10 @@ gen_elem_of_pack_expansion_instantiation (tree pattern,
       ARGUMENT_PACK_SELECT_INDEX (aps) = index;
     }
 
+  // Any local specialization bindings arising from this substitution
+  // cannot be reused for a different INDEX.
+  local_specialization_stack lss (lss_copy);
+
   /* Substitute into the PATTERN with the (possibly altered)
      arguments.  */
   if (pattern == in_decl)
@@ -17912,8 +17916,17 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
   tree oldfn = lambda_function (t);
   in_decl = oldfn;
 
+  /* If we have already specialized this lambda expr, reuse it.  See
+     PR c++/87322.  */
+  if (local_specializations)
+    if (tree r = retrieve_local_specialization (t))
+      return r;
+
   tree r = build_lambda_expr ();
 
+  if (local_specializations)
+    register_local_specialization (r, t);
+
   LAMBDA_EXPR_LOCATION (r)
     = LAMBDA_EXPR_LOCATION (t);
   LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (r)
@@ -18005,6 +18018,11 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
     r = error_mark_node;
   else
     {
+      /* The body of a lambda-expression is not a subexpression of the
+	 enclosing expression.  Parms are to have DECL_CHAIN tsubsted,
+	 which would be skipped if cp_unevaluated_operand.  */
+      cp_evaluated ev;
+
       /* Fix the type of 'this'.  */
       fntype = build_memfn_type (fntype, type,
 				 type_memfn_quals (fntype),
@@ -18026,10 +18044,6 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
       /* Let finish_function set this.  */
       DECL_DECLARED_CONSTEXPR_P (fn) = false;
 
-      /* The body of a lambda-expression is not a subexpression of the
-	 enclosing expression.  */
-      cp_evaluated ev;
-
       bool nested = cfun;
       if (nested)
 	push_function_context ();
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C
index 97f64cd761a7..1f931757b72f 100644
--- a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C
@@ -1,5 +1,7 @@
 // PR c++/47226
 // { dg-do compile { target c++11 } }
+// { dg-options "-fdump-tree-original" }
+// { dg-final { scan-tree-dump-times "::<lambda\\(\\)> \\(null\\)" 6 "original" } }
 
 template<class T>
 void print(const T&) {}
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr87322.C b/gcc/testsuite/g++.dg/cpp1y/pr87322.C
new file mode 100644
index 000000000000..8f52e0e3b99b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr87322.C
@@ -0,0 +1,23 @@
+// { dg-do compile { target c++14 } }
+
+#include <array>
+#include <algorithm>
+
+int main()
+{
+  constexpr std::array<std::array<double,2>,3> my_mat { 
+     { { 1., 1. },
+       { 1., 1. },
+       { 1., 1. }, }
+  };
+  
+  std::for_each(my_mat.begin(), my_mat.end(), [
+      inner_func =  [] (auto a, auto b) { return a + b; } ](auto& row) {
+    std::for_each(row.begin(), row.end(), [&,
+      inner_func2 =  [] (auto a, auto b) { return a + b; } ]
+      (const double&) {
+        return;
+    });
+  }); 
+  
+}


-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Jason Merrill Feb. 8, 2019, 10:22 p.m. | #3
On 2/8/19 1:58 AM, Alexandre Oliva wrote:
> On Feb  7, 2019, Jason Merrill <jason@redhat.com> wrote:

> 

>>> +     PR c++/86322.  */

> 

>> Wrong PR number.

> 

> Thanks

> 

>>> +  if (local_specializations)

>>> +    if (tree r = retrieve_local_specialization (t))

>>> +      return r;

> 

>> Hmm, I would expect this to do the wrong thing for pack expansion of a

>> lambda, giving us only one rather than one per element of the

>> expansion.

> 

> Oooh.  Indeed, I hadn't realized this was possible.

> 

> I think we should separate local_specializations arising from different

> pack expansion bindings, considering they won't always appear in tsubst

> args:

> 

> 

> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c

> index 0177d7f77891..9f954698f454 100644

> --- a/gcc/cp/pt.c

> +++ b/gcc/cp/pt.c

> @@ -11698,6 +11698,10 @@ gen_elem_of_pack_expansion_instantiation (tree pattern,

>         ARGUMENT_PACK_SELECT_INDEX (aps) = index;

>       }

>   

> +  // Any local specialization bindings arising from this substitution

> +  // cannot be reused for a different INDEX.

> +  local_specialization_stack lss (lss_copy);

> +

>     /* Substitute into the PATTERN with the (possibly altered)

>        arguments.  */

>     if (pattern == in_decl)

> 

> 

>> For instance, in lambda-variadic5.C we should get three

>> instantiations of the "print" function template; can you add that

>> check to the test?

> 

> This causes lambda-variadic5.C to get 6 separate lambdas, instead of 2,

> so it passes after the following patchlet:

> 

> diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C

> index 97f64cd761a7..1f931757b72f 100644

> --- a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C

> +++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-variadic5.C

> @@ -1,5 +1,7 @@

>   // PR c++/47226

>   // { dg-do compile { target c++11 } }

> +// { dg-options "-fdump-tree-original" }

> +// { dg-final { scan-tree-dump-times "::<lambda\\(\\)> \\(null\\)" 6 "original" } }

>   

>   template<class T>

>   void print(const T&) {}

> 

> 

> Now, should I adjust lambda-variadic5.C to check for the presence of the

> expencted count of lambdas in compiler dumps?


Sounds good.

> Thinking of how to test for it drew my attention to issues about lambda

> naming, that may have implications on ABI and duplicate code removal.

> There are two issues I'm concerned with:

> 

> - we increment lambda_cnt when introducing a lambda within a generic,

>    and then increment it again as we specialize, partially or fully, each

>    lambda expr/object/type set.  This makes for gaps in numbering of full

>    specializations and, being a global counter, does not ensure the

>    assignment of the same symbol name to corresponding instantiations in

>    separate translation units


We shouldn't use that name in any symbol shared between translation units.

> - we don't seem to attempt to reuse lambdas across different

>    specializations of argument pack expansions.


Right, we shouldn't reuse them (other than via identical code folding 
optimizations).

>    I guess that's ok, we

>    don't attempt to reuse them across different specializations of the

>    enclosing template function or class either, but, unlike enclosing

>    contexts, different argument pack indices do not mandate different

>    mangled names.  This came up because I was thinking of having a

>    sub-map in local_specializations, so that the lambda would map to

>    another map in which we could look up template arguments to find a

>    specific instance.  That wouldn't work, in part because args does not

>    provide the entire set of substitutions, and in another part because

>    we don't seem to be able to identify, in patterns undergoing

>    substitution but before actually performing the substitution, the set

>    of (implied?) template arguments that might affect the substitution

>    result.


Indeed.  When I was fixing lambdas in pack expansions I thought about 
making lambdas in pack expansions implicit templates with the parameters 
matching the packs in the pack expansion, but the current implementation 
matches the specification better I think.

> Here's the patch I'm testing.  OK if it passes?


OK.

Jason

Patch

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index b8fbf4046f07..3dffa8d2de88 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -17912,8 +17912,17 @@  tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
   tree oldfn = lambda_function (t);
   in_decl = oldfn;
 
+  /* If we have already specialized this lambda expr, reuse it.  See
+     PR c++/86322.  */
+  if (local_specializations)
+    if (tree r = retrieve_local_specialization (t))
+      return r;
+
   tree r = build_lambda_expr ();
 
+  if (local_specializations)
+    register_local_specialization (r, t);
+
   LAMBDA_EXPR_LOCATION (r)
     = LAMBDA_EXPR_LOCATION (t);
   LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (r)
@@ -18005,6 +18014,11 @@  tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
     r = error_mark_node;
   else
     {
+      /* The body of a lambda-expression is not a subexpression of the
+	 enclosing expression.  Parms are to have DECL_CHAIN tsubsted,
+	 which would be skipped if cp_unevaluated_operand.  */
+      cp_evaluated ev;
+
       /* Fix the type of 'this'.  */
       fntype = build_memfn_type (fntype, type,
 				 type_memfn_quals (fntype),
@@ -18026,10 +18040,6 @@  tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
       /* Let finish_function set this.  */
       DECL_DECLARED_CONSTEXPR_P (fn) = false;
 
-      /* The body of a lambda-expression is not a subexpression of the
-	 enclosing expression.  */
-      cp_evaluated ev;
-
       bool nested = cfun;
       if (nested)
 	push_function_context ();
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr87322.C b/gcc/testsuite/g++.dg/cpp1y/pr87322.C
new file mode 100644
index 000000000000..8f52e0e3b99b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr87322.C
@@ -0,0 +1,23 @@ 
+// { dg-do compile { target c++14 } }
+
+#include <array>
+#include <algorithm>
+
+int main()
+{
+  constexpr std::array<std::array<double,2>,3> my_mat { 
+     { { 1., 1. },
+       { 1., 1. },
+       { 1., 1. }, }
+  };
+  
+  std::for_each(my_mat.begin(), my_mat.end(), [
+      inner_func =  [] (auto a, auto b) { return a + b; } ](auto& row) {
+    std::for_each(row.begin(), row.end(), [&,
+      inner_func2 =  [] (auto a, auto b) { return a + b; } ]
+      (const double&) {
+        return;
+    });
+  }); 
+  
+}