Re: [C++ PATCH] Fix lambda capture duplicate handling (PR c++/89767, take 3 & 4)
On 3/20/19 1:15 PM, Jakub Jelinek wrote: On Wed, Mar 20, 2019 at 11:10:19AM -0400, Nathan Sidwell wrote: I was unclear. I was for the lazy creation of the hash. I just think it can be lazily created at either the first or second explicit capture. On top of the https://gcc.gnu.org/ml/gcc-patches/2019-03/msg00991.html patch I've just posted here are two different patches to do this, the first one optimizes just the zero captures case and creates a hash_set on the first add_capture, the second one optimizes zero and one captures case and creates a hash_set on the second add_capture. Thanks. Looking at the 8 or so lines in the second patch dealing with the single case I can't help thinking that's not an optimization. I suppose the initial calloc could be pricy. But anyway, overthinking this. Either patch is fine -- you choose! nathan -- Nathan Sidwell
Re: [C++ PATCH] Fix lambda capture duplicate handling (PR c++/89767)
On 3/20/19 10:48 AM, Jakub Jelinek wrote: On Wed, Mar 20, 2019 at 10:34:51AM -0400, Nathan Sidwell wrote: On 3/19/19 2:14 PM, Jakub Jelinek wrote: add_capture when parsing a lambda introducer uses the IDENTIFIER_MARKED bit to detect duplicate captures. I guess in strict C++11 that could have worked, if the introducer could contain just identifiers, but in C++14 it has 2 problems: The following patch stops using IDENTIFIER_MARKED for this and uses a hash_set instead. But, as I believe lambdas with 0 (or very few) explicit captures are extremely common, I've tried not to slow down those with allocation of a hash_set and deallocating it again, so it uses a linear search if there are up to 8 captures and starts using a hash_set only when getting above that count. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Or do you think allocation of an hash_set and deallocation would be in a noise? I think it'd be in the noise. The high bit is between zero and more than zero captures. The next most common after zero would be 1. Besides, identifiers are trivial to hash. What I meant is that even just hash_set var; implies a xcalloc (13, sizeof (void*)) or so and destroying it a free of that. So, if 90% of all lambdas in the headers have zero captures and 5% have 1, then simplifying the patch by adding that hash_set var; into the lambda introducer parsing routine and just always using the hash set would add those to the parsing of all those 95%+ lambda parsings that don't really need that. Here is attached completely untested variant that does that unconditional xcalloc/free for every lambda introducer parsing. I was unclear. I was for the lazy creation of the hash. I just think it can be lazily created at either the first or second explicit capture. nathan -- Nathan Sidwell
Re: [C++ PATCH] Fix lambda capture duplicate handling (PR c++/89767)
On Wed, Mar 20, 2019 at 10:34:51AM -0400, Nathan Sidwell wrote:
> On 3/19/19 2:14 PM, Jakub Jelinek wrote:
> > add_capture when parsing a lambda introducer uses the IDENTIFIER_MARKED
> > bit to detect duplicate captures.
> > I guess in strict C++11 that could have worked, if the introducer could
> > contain just identifiers, but in C++14 it has 2 problems:
>
> > The following patch stops using IDENTIFIER_MARKED for this and uses a
> > hash_set instead. But, as I believe lambdas with 0 (or very few) explicit
> > captures are extremely common, I've tried not to slow down those with
> > allocation of a hash_set and deallocating it again, so it uses
> > a linear search if there are up to 8 captures and starts using a hash_set
> > only when getting above that count.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > Or do you think allocation of an hash_set and deallocation would be in a
> > noise?
>
> I think it'd be in the noise. The high bit is between zero and more than
> zero captures. The next most common after zero would be 1. Besides,
> identifiers are trivial to hash.
What I meant is that even just hash_set var; implies a xcalloc (13,
sizeof (void*)) or so and destroying it a free of that.
So, if 90% of all lambdas in the headers have zero captures and 5% have 1,
then simplifying the patch by adding that hash_set var; into the
lambda introducer parsing routine and just always using the hash set would
add those to the parsing of all those 95%+ lambda parsings that don't really
need that.
Here is attached completely untested variant that does that unconditional
xcalloc/free for every lambda introducer parsing.
The previously posted patch was:
cp/cp-tree.h |3 +
cp/lambda.c| 54 -
cp/parser.c| 10 --
this variant is:
cp/cp-tree.h |3 ++-
cp/lambda.c| 14 +++---
cp/parser.c|7 ---
2019-03-20 Jakub Jelinek
PR c++/89767
* cp-tree.h (add_capture): Add ids argument.
* parser.c (cp_parser_lambda_introducer): Add ids variable and pass
its address to add_capture calls.
* lambda.c (add_capture): Add ids argument, don't use
IDENTIFIER_MARKED, instead use ids hash_set for that.
(register_capture_members): Don't clear IDENTIFIER_MARKED here.
(add_default_capture): Adjust add_capture caller.
* g++.dg/cpp1y/lambda-init18.C: New test.
* g++.dg/cpp1y/lambda-init19.C: New test.
* g++.dg/cpp1y/pr89767.C: New test.
--- gcc/cp/cp-tree.h.jj 2019-03-19 17:10:03.135143659 +0100
+++ gcc/cp/cp-tree.h2019-03-20 15:41:53.561214488 +0100
@@ -7150,7 +7150,8 @@ extern tree lambda_return_type(tree);
extern tree lambda_proxy_type (tree);
extern tree lambda_function(tree);
extern void apply_deduced_return_type (tree, tree);
-extern tree add_capture (tree, tree, tree, bool, bool);
+extern tree add_capture (tree, tree, tree, bool, bool,
+hash_set *);
extern tree add_default_capture (tree, tree, tree);
extern void insert_capture_proxy (tree);
extern void insert_pending_capture_proxies (void);
--- gcc/cp/parser.c.jj 2019-03-19 17:10:03.074144632 +0100
+++ gcc/cp/parser.c 2019-03-20 15:42:44.165434239 +0100
@@ -10547,6 +10547,7 @@ cp_parser_lambda_introducer (cp_parser*
error ("non-local lambda expression cannot have a capture-default");
}
+ hash_set ids;
while (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_SQUARE))
{
cp_token* capture_token;
@@ -10586,7 +10587,7 @@ cp_parser_lambda_introducer (cp_parser*
/*id=*/this_identifier,
/*initializer=*/finish_this_expr (),
/*by_reference_p=*/true,
- explicit_init_p);
+ explicit_init_p, );
continue;
}
@@ -10604,7 +10605,7 @@ cp_parser_lambda_introducer (cp_parser*
/*id=*/this_identifier,
/*initializer=*/finish_this_expr (),
/*by_reference_p=*/false,
- explicit_init_p);
+ explicit_init_p, );
continue;
}
@@ -10757,7 +10758,7 @@ cp_parser_lambda_introducer (cp_parser*
capture_id,
capture_init_expr,
/*by_reference_p=*/capture_kind == BY_REFERENCE,
- explicit_init_p);
+ explicit_init_p, );
/* If there is any qualification still in effect, clear it
now; we will be starting fresh with the next capture. */
--- gcc/cp/lambda.c.jj
Re: [C++ PATCH] Fix lambda capture duplicate handling (PR c++/89767)
On 3/19/19 2:14 PM, Jakub Jelinek wrote: Hi! add_capture when parsing a lambda introducer uses the IDENTIFIER_MARKED bit to detect duplicate captures. I guess in strict C++11 that could have worked, if the introducer could contain just identifiers, but in C++14 it has 2 problems: The following patch stops using IDENTIFIER_MARKED for this and uses a hash_set instead. But, as I believe lambdas with 0 (or very few) explicit captures are extremely common, I've tried not to slow down those with allocation of a hash_set and deallocating it again, so it uses a linear search if there are up to 8 captures and starts using a hash_set only when getting above that count. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Or do you think allocation of an hash_set and deallocation would be in a noise? I think it'd be in the noise. The high bit is between zero and more than zero captures. The next most common after zero would be 1. Besides, identifiers are trivial to hash. nathan -- Nathan Sidwell
