ilya-biryukov added inline comments.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2042
+ !SemaRef.CheckConstraintExpression(TransConstraint.get())) {
+ assert(Trap.hasErrorOccurred() && "CheckConstraintExpression failed, but
"
+ "did not produce a SFINAE error");
----------------
erichkeane wrote:
> ilya-biryukov wrote:
> > erichkeane wrote:
> > > ilya-biryukov wrote:
> > > > erichkeane wrote:
> > > > > ilya-biryukov wrote:
> > > > > > erichkeane wrote:
> > > > > > > This branch ends up being empty if asserts are off. Also, it
> > > > > > > results in CheckConstraintExpression happening 2x, which ends up
> > > > > > > being more expensive after https://reviews.llvm.org/D126907
> > > > > > > This branch ends up being empty if asserts are off. Also, it
> > > > > > > results in CheckConstraintExpression happening 2x, which ends up
> > > > > > > being more expensive after https://reviews.llvm.org/D126907
> > > > > >
> > > > > > Yeah, good point, I have update it.
> > > > > >
> > > > > > I am not sure why would `CheckConstraintExpression` be called
> > > > > > twice, could you elaborate? Note that we do not call
> > > > > > `BuildNestedRequirement` anymore and use placement new directly to
> > > > > > avoid extra template instantiations. Instead we call
> > > > > > `CheckConstraintExpression` directly to account for any errors.
> > > > > This check does not seem to cause a 'return' to the function, but
> > > > > then falls through to the version on 2052, right?
> > > > >
> > > > > `CheckConstraintExpression`/`CheckConstraintSatisfaction`(i think?)
> > > > > ends up now having to re-instantiate every time it is called, so any
> > > > > ability to cache results ends up being beneficial here.
> > > > The number of calls to these functions is actually the same.
> > > >
> > > > `CheckConstraintExpression` used to be called during
> > > > `CheckConstraintSatisfaction` (that does instantiations) for every
> > > > atomic constraint after the substitution. It merely checks that each
> > > > constraint have a bool type and does not do any substitutions, so it's
> > > > pretty cheap anyway.
> > > >
> > > > `CheckConstraintSatisfaction` was called inside
> > > > `BuildNestedRequirement`, we now call a different overload here
> > > > directly that does not do any extra template instantiations directly.
> > > >
> > > > That way we end up doing the same checks without running recursive
> > > > template instantiations.
> > > Hmm... I guess what I'm saying is: I would like it if we could
> > > minimize/reduce the calls to calcuateConstraintSatisfaction (in
> > > SemaConcept.cpp) that does the call to SubstConstraintExpr (or
> > > substExpr).
> > >
> > > That ends up being more expensive thanks to my other patch.
> > Ah, yes, that makes sense. Note that `CheckConstraintSatisfaction` call on
> > 2052 does not do any substitutions, it merely evaluates the expressions if
> > they are not dependent.
> >
> > I will have to look more closely into your patch to get a sense of why
> > substituting to the constraint expressions is more expensive after it.
> >
> > BTW, I was wondering if there is any reason to substitute empty template
> > arguments to evaluate non-dependent constraints? (This is what
> > `CalculateConstraintSatisfaction` does)
> > I feels like merely evaluating those should be enough.
> ah, I see! Constraint expressions get 'more expensive' because now we
> actually have to do substitution EVERY time we evaluate, rather than just
> having an already non-dependent expression stored. This is because the
> deferred constraint instantiation requires that we re-evaluate it just about
> every time we check a constraint.
>
> Which version of `CalculateConstraintSatisfaction` are you referring to? it
> DOES actually appear that the one you modify above doesn't actually DO any
> substitution. it DOES looklike it does some checking to make sure we
> specifically are a boolean expr ([temp.constr.atompic]p1 ~ line 210), but it
> isn't clear to me what the logic there is either.
> This is because the deferred constraint instantiation requires that we
> re-evaluate it just about every time we check a constraint.
That makes sense. And this is to meet the requirements of the standard, right?
Reading through it, we should keep "template parameter mappings" and original
expressions for atomic constraint and only do the substitution once we actually
need to check the constraint.
PS I should really read the code first to understand what it's doing, but
hopefully I'm on the right track.
> Which version of CalculateConstraintSatisfaction are you referring to?
Sorry, got confused here. I meant the overload of `CheckConstraintSatisfaction`
that does empty template argument substitution. I was wondering whether we need
template instantiations there in the first place. It looks like it might be
there for code reuse and we can instead just evaluate the expressions directly.
But maybe I'm missing something.
As for the overload of `CheckConstraintSatisfaction` that does no substitutions.
> looklike it does some checking to make sure we specifically are a boolean
> expr.
The check for boolean type is done by `CheckConstraintExpression` during
instantiation. However, there is a change in the body of the function to add
lvalue-to-rvalue conversion. This is necessary to make sure the code inside
`calculateConstraintSatisfaction` gets correct evaluation results. This
overload of `CheckConstraintExpression` happened to be broken in that regard,
but it was not widely used (as all constraints normally get evaluated by the
other overload), so this was never caught this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127487/new/
https://reviews.llvm.org/D127487
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
