Re: [PATCH] Giant concepts patch
On Fri, Mar 25, 2016 at 1:33 AM, Andrew Sutton wrote: > I'll just leave this here... > > This patch significantly improves performance with concepts (i.e., > makes it actually usable for real systems) and improves the > specificity of diagnostics when constraints fail. > > Unfortunately, this isn't easily submittable in small pieces because > it completely replaces most of the core processing routines for > constraints, including (essentially) a complete rewrite of logic.cc > and the diagnostics in constraint.cc. More perfective work could be > done related to diagnostics, but this needs to be applied first. > > As part of the patch, I added timevars for constraint satisfaction and > subsumption. In template-heavy coe (~80KLOC), I'm seeing satisfaction > account for ~6% of compilation time and subsumption ~2%. Template > instantiation remains ~35%, but I think there's still room for > improvement in concepts. It just requires experimentation. > > Tests involving significant number of disjunctions have yet to > register > 1% of compilation time spent in subsumption, but I'm still > testing. Thanks, I've been working on integrating this patch, hoping to have it in for 6.2. Have you done more work on it since you sent this out? A few issues: I've run into some trouble building cmcstl2: declarator requirements on a function can lead to constraints that tsubst_constraint doesn't handle. What was your theory of only handling a few _CONSTR codes there? This is blocking me from checking in the patch. Adding gcc_unreachable to the ARGUMENT_PACK_SELECT handling in concept arg hash/compare doesn't seem to break anything in either the GCC testsuite or your stl2. Do you have a testcase where that code is still needed? > Also, it might be worth noting that partial specialization of variable > templates is currently broken. We don't seem to be emitting template > arguments as part of the mangled name, leading to lots and lots of > late redefinition errors. This should be fixed now. Jason
Re: [PATCH] Giant concepts patch
I've pushed my work-in-progress integration branch to jason/concepts-rewrite. Jason On Mon, Jun 20, 2016 at 4:28 PM, Jason Merrill wrote: > On Fri, Mar 25, 2016 at 1:33 AM, Andrew Sutton > wrote: >> I'll just leave this here... >> >> This patch significantly improves performance with concepts (i.e., >> makes it actually usable for real systems) and improves the >> specificity of diagnostics when constraints fail. >> >> Unfortunately, this isn't easily submittable in small pieces because >> it completely replaces most of the core processing routines for >> constraints, including (essentially) a complete rewrite of logic.cc >> and the diagnostics in constraint.cc. More perfective work could be >> done related to diagnostics, but this needs to be applied first. >> >> As part of the patch, I added timevars for constraint satisfaction and >> subsumption. In template-heavy coe (~80KLOC), I'm seeing satisfaction >> account for ~6% of compilation time and subsumption ~2%. Template >> instantiation remains ~35%, but I think there's still room for >> improvement in concepts. It just requires experimentation. >> >> Tests involving significant number of disjunctions have yet to >> register > 1% of compilation time spent in subsumption, but I'm still >> testing. > > Thanks, I've been working on integrating this patch, hoping to have it > in for 6.2. Have you done more work on it since you sent this out? > > A few issues: > > I've run into some trouble building cmcstl2: declarator requirements > on a function can lead to constraints that tsubst_constraint doesn't > handle. What was your theory of only handling a few _CONSTR codes > there? This is blocking me from checking in the patch. > > Adding gcc_unreachable to the ARGUMENT_PACK_SELECT handling in concept > arg hash/compare doesn't seem to break anything in either the GCC > testsuite or your stl2. Do you have a testcase where that code is > still needed? > >> Also, it might be worth noting that partial specialization of variable >> templates is currently broken. We don't seem to be emitting template >> arguments as part of the mangled name, leading to lots and lots of >> late redefinition errors. > > This should be fixed now. > > Jason
Re: [PATCH] Giant concepts patch
On Wed, Jun 22, 2016 at 2:25 AM, Andrew Sutton wrote: >> > I've run into some trouble building cmcstl2: declarator requirements >> > on a function can lead to constraints that tsubst_constraint doesn't >> > handle. What was your theory of only handling a few _CONSTR codes >> > there? This is blocking me from checking in the patch. > > I wonder if those were the problems that I was running into, but hadn't > diagnosed. I had thought it shouldn't be possible to get the full set of > constraints in tsubst_constraint. I may have mis-analyzed the problem for > function constraints. Any further thoughts? Jason
Re: [PATCH] Giant concepts patch
I just tried building a fresh pull of cmcstl2, and I'm not seeing any
errors as a result of not handling those missing codes in
tsubst_constraint. At one point, I think it was not possible to get
those other constraints in this context because they were nested in a
parm_constr. But that seems obviously untrue now. But still... that
gcc_unreachable isn't being triggered by any code in cmcstl.
I attached a patch that adds tsubsts for the missing constraints.
Unfortunately, I don't have time to test thoroughly today.
I did find another bug building cmcstl2, hence the attached
disable-opt patch. For some reason, the memoization of concept
satisfaction is giving momoized results for concept + args that have
not yet been evaluated. This is exactly the same problem that made me
disable the lookup/memoize_constraint_sat optimizations. Somehow I'm
getting the same hash code for different arguments, and they also
happen to compare equal.
This doesn't seem to affect memoization of concept satisfaction. At
least I haven't seen it yet.
Anyways, disabling that optimization lets me build cmcstl2 with
concepts. Sort of; there's a bug in the library, which is why Casey is
added to the mailing. You're missing a const overload of operator* for
basic_operator. Patch forthcoming.
Changelogs below.
2016-07-10 Andrew Sutton
* constraint.cc (tsubst_type_constr, tsubst_implicit_conversion_constr,
tsubst_argument_deduction_constr, tsubst_exception_constr): New.
(tsubst_constraint): Add cases for missing constraints.
2016-07-10 Andrew Sutton
* pt.c (lookup_concept_satisfaction, memoize_concept_satisfaction):
Disable memoization of concept results.
Andrew Sutton
On Sat, Jul 9, 2016 at 11:24 AM, Andrew Sutton
wrote:
> Do we have a smaller test that reproduces this? One reason I didn't make
> much progress was that I could never find a small test that triggers the
> problem. I just pulled your branch and plan to do some digging tomorrow.
>
>
>
> On Fri, Jul 8, 2016 at 6:42 PM Jason Merrill wrote:
>>
>> On Wed, Jun 22, 2016 at 2:25 AM, Andrew Sutton
>> wrote:
>> >> > I've run into some trouble building cmcstl2: declarator requirements
>> >> > on a function can lead to constraints that tsubst_constraint doesn't
>> >> > handle. What was your theory of only handling a few _CONSTR codes
>> >> > there? This is blocking me from checking in the patch.
>> >
>> > I wonder if those were the problems that I was running into, but hadn't
>> > diagnosed. I had thought it shouldn't be possible to get the full set of
>> > constraints in tsubst_constraint. I may have mis-analyzed the problem
>> > for
>> > function constraints.
>>
>> Any further thoughts?
>>
>> Jason
>
> --
> Andrew Sutton
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 145ae1e..745cbbc 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -1625,12 +1625,70 @@ static tree
tsubst_expr_constr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
{
cp_unevaluated guard;
-
tree expr = EXPR_CONSTR_EXPR (t);
- tree check = tsubst_expr (expr, args, complain, in_decl, false);
- if (check == error_mark_node)
+ tree ret = tsubst_expr (expr, args, complain, in_decl, false);
+ if (ret == error_mark_node)
+return error_mark_node;
+ return build_nt (EXPR_CONSTR, ret);
+}
+
+static tree
+tsubst_type_constr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
+{
+ tree type = TYPE_CONSTR_TYPE (t);
+ tree ret = tsubst (type, args, complain, in_decl);
+ if (ret == error_mark_node)
return error_mark_node;
- return build_nt (EXPR_CONSTR, check);
+ return build_nt (TYPE_CONSTR, ret);
+}
+
+static tree
+tsubst_implicit_conversion_constr (tree t, tree args, tsubst_flags_t complain,
+ tree in_decl)
+{
+ cp_unevaluated guard;
+ tree expr = ICONV_CONSTR_EXPR (t);
+ tree type = ICONV_CONSTR_TYPE (t);
+ tree new_expr = tsubst_expr (expr, args, complain, in_decl, false);
+ if (new_expr == error_mark_node)
+return error_mark_node;
+ tree new_type = tsubst (type, args, complain, in_decl);
+ if (new_type == error_mark_node)
+return error_mark_node;
+ return build_nt (ICONV_CONSTR, new_expr, new_type);
+}
+
+static tree
+tsubst_argument_deduction_constr (tree t, tree args, tsubst_flags_t complain,
+ tree in_decl)
+{
+ cp_unevaluated guard;
+ tree expr = DEDUCT_CONSTR_EXPR (t);
+ tree pattern = DEDUCT_CONSTR_PATTERN (t);
+ tree autos = DEDUCT_CONSTR_PLACEHOLDER(t);
+ tree new_expr = tsubst_expr (expr, args, complain, in_decl, false);
+ if (new_expr == error_mark_node)
+return error_mark_node;
+ /* It seems like substituting through the pattern will not affect the
+ placeholders. We should (?) be able to reuse the existing list
+ without any problems. If not, then we probably want to create a
+ new list of placeholders and then instantiate the pattern using
+ those. */
+ tree new_pa
Re: [PATCH] Giant concepts patch
Ah sure. Jason has been vetting my post-Jacksonville concepts patch in the branch jason/concepts-rewrite. I just pulled this off the github GCC mirror this morning to look at an outstanding question. Resulted in the previous 2 patches. I tried building a fresh pull of your cmcstl2 and got an off-by const error. It looks like it's coming from counted_iterator. I'll post the repro instructions and the error on the bug report. Andrew
Re: [PATCH] Giant concepts patch
On Sun, Jul 10, 2016 at 11:20 AM, Andrew Sutton
wrote:
> I just tried building a fresh pull of cmcstl2, and I'm not seeing any
> errors as a result of not handling those missing codes in
> tsubst_constraint. At one point, I think it was not possible to get
> those other constraints in this context because they were nested in a
> parm_constr. But that seems obviously untrue now. But still... that
> gcc_unreachable isn't being triggered by any code in cmcstl.
The only one that was triggered by cmcstl was EXPR_CONSTR, and then
only for a member; if you comment out the EXPR_CONSTR case that I
added to tsubst_constraint, this test will ICE.
struct B
{
template void f(T t)
requires requires (T tt) { tt; }
{ }
};
int main()
{
B().f(42);
}
Jason
Re: [PATCH] Giant concepts patch
On Sun, Jul 10, 2016 at 11:20 AM, Andrew Sutton wrote: > I did find another bug building cmcstl2, hence the attached > disable-opt patch. For some reason, the memoization of concept > satisfaction is giving momoized results for concept + args that have > not yet been evaluated. This is exactly the same problem that made me > disable the lookup/memoize_constraint_sat optimizations. Somehow I'm > getting the same hash code for different arguments, and they also > happen to compare equal. This bug turned out to be e.g. substituting int into "requires C", which fails because int has no foo member, and therefore deciding that C is false. After I fixed that, I tried turning on the constraint memos, which didn't seem to break anything. I've pushed to the jason/concepts-rewrite branch again. See any reason I shouldn't merge to trunk now? Jason
