ilya-biryukov added inline comments.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:352
+ [this](const Expr *AtomicExpr) -> ExprResult {
+ // We only do this to immitate lvalue-to-rvalue conversion.
+ return PerformContextuallyConvertToBool(const_cast<Expr*>(AtomicExpr));
----------------
erichkeane wrote:
> ilya-biryukov wrote:
> > royjacobson wrote:
> > > erichkeane wrote:
> > > > ilya-biryukov wrote:
> > > > > erichkeane wrote:
> > > > > > Can you explain this more? How does this work, and why don't we do
> > > > > > that directly instead?
> > > > > That's entangled with `calculateConstraintSatisfaction`. I actually
> > > > > tried to do it directly, but before passing expressions to this
> > > > > function `calculateConstraintSatisfaction` calls
> > > > > `IgnoreParenImpCasts()`, which strips away the lvalue-to-rvalue
> > > > > conversion.
> > > > > And we need this conversion so that the evaluation that runs after
> > > > > this callback returns actually produces an r-value.
> > > > >
> > > > > Note that the other call to `calculateConstraintSatisfaction` also
> > > > > calls `PerformContextuallyConvertToBool` after doing template
> > > > > substitution into the constraint expression.
> > > > >
> > > > > I don't have full context on why it's the way it is, maybe there is a
> > > > > more fundamental change that helps with both cases.
> > > > Hmm... my understanding is we DO need these to be a boolean expression
> > > > eventually, since we have to test them as a bool, so that is why the
> > > > other attempts the converesion. If you think of any generalizations of
> > > > this, it would be appreciated, I'll think it through as well.
> > > Note we already have a related bug about this
> > > https://github.com/llvm/llvm-project/issues/54524
> > Yeah, they have to be bool and we actually check for that in
> > `CheckConstraintExpression`. The standard explicitly mentions only the
> > lvalue-to-rvalue conversion should be performed.
> > ```
> > [temp.constr.atomic]p3 If substitution results in an invalid type or
> > expression, the constraint is
> > not satisfied. Otherwise, the lvalue-to-rvalue conversion (7.3.1) is
> > performed if necessary, and E shall be a constant expression of type bool.
> > ```
> >
> > However, in the calls to `calculateConstraintSatisfaction` we do a more
> > generic boolean conversion, but the comment in the other call site suggests
> > this probably accidental and we actually want a less generic conversion:
> > ```
> > // Substitution might have stripped off a contextual conversion to
> > // bool if this is the operand of an '&&' or '||'. For example, we
> > // might lose an lvalue-to-rvalue conversion here. If so, put it
> > back
> > // before we try to evaluate.
> > if (!SubstitutedExpression.isInvalid())
> > SubstitutedExpression =
> >
> > S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
> > ```
> >
> > I am happy to take a look at fixing the mentioned bug.
> Hmm... yeah, I find myself wondering if there is a better lval/rval
> conversion function here, and I'm guessing the contextually convert to bool
> is the wrong one.
Yep, it's definitely the wrong function. I could not find a better one, but I
only briefly looked through the options.
I will have to find one or implement it while fixing
https://github.com/llvm/llvm-project/issues/54524.
================
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:
> > > > > 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.
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
