[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3591195 <https://reviews.llvm.org/D126907#3591195>, @ChuanqiXu wrote:

> In D126907#3588708 <https://reviews.llvm.org/D126907#3588708>, @erichkeane wrote:
>
>> In D126907#3588417 <https://reviews.llvm.org/D126907#3588417>, @ChuanqiXu wrote:
>>
>>> From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In `::CheckConstraintSatisfaction`, the depth of `RawCompletionToken ` is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.
>>>
>>> The first though was that we should set the depth of `RawCompletionToken ` to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of `RawCompletionToken ` is 0 indeed. What is different that time is the depth of corresponding `MultiLevelTemplateArgumentList ` is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.
>>>
>>> So I feel like what we should do here is in `Sema::CheckInstantiatedFunctionTemplateConstraints`, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.
>>>
>>> ---
>>>
>>> Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.
>>
>> I don't know of the 'another idea'?  We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation.  And the problem is that the RawCompletionToken uses a concept to refer to 'itself'.  However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'.  However, this IS happening during instantiation?
>>
>> My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't?  I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.
>
> Yeah, we have to do instantiation before checking. My point is that it looks like we're doing **another** instantiation when we check the concepts. And my point is that if it we should avoid the second instantiation.

Hmm... yeah, I think we actually want to skip the FIRST instantiation?  I think the work I did above in Sema for `IsEvaluatingAConstraint` was too greedy, it ends up instantiating constraints on the 'while we are there' type things, rather than the things being currently checked.  I found that if I can make it a state of the instantiators themselves, it seems to work, at least for your example.  I've got it down to 1 'lit' test failure, but have been looking at the other problem first (below).

The Github-Issue crash IS a regression from this patch, and I've minimized it sufficiently.  I believe I have a hold on how to fix it, I just have to do so :)  If I make any further progress, I'll clean this up and put it up here, even if it DOES have the lit test failure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126907/new/

https://reviews.llvm.org/D126907