[PATCH] D127487: [Sema] Fix assertion failure when instantiating requires expression

[Ilya Biryukov via Phabricator via cfe-commits]

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