erichkeane added a comment. In D127487#3573667 <https://reviews.llvm.org/D127487#3573667>, @ilya-biryukov wrote:
> I've checked that the change works fine on top of D126907 > <https://reviews.llvm.org/D126907>. The bug is still there after D126907 > <https://reviews.llvm.org/D126907> and gets fixed by this. > Also, the merge conflict is actually minimal, no code changes intersect. Thanks for looking into this! I DID see it was still in place after the fact, but wanted to make sure this wouldn't break any more of the tests. ================ 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)); ---------------- 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. ================ Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2042 + !SemaRef.CheckConstraintExpression(TransConstraint.get())) { + assert(Trap.hasErrorOccurred() && "CheckConstraintExpression failed, but " + "did not produce a SFINAE error"); ---------------- 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. 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
