[PATCH] D131858: [clang] Track the templated entity in type substitution.

Sat Oct 8 17:29:25 PDT 2022

mizvekov added inline comments.

================
Comment at: clang/include/clang/AST/Type.h:1836
+    // The index of the template parameter this substitution represents.
+    unsigned Index : 15;
+
----------------
erichkeane wrote:
> Is it a problem for this to be a different number of bits used to represent the number of template parameters?  I would expect we would want to make them all them the same size (else we have an additional limit on the size of parameters).
Per documentation reasons, it would certainly be simpler if they were the same size.

But they are not the same thing. Index is for indexing into arguments bound to non-pack parameters,
while PackIndex is for indexing arguments within a pack.

The only way Index could reach such huge values, would be if there were a corresponding huge amount of template parameters. For a pack that is not the case.

So packs are much more efficient for handling large amounts of arguments, so it makes sense to have a higher limit for them.

But how much that should be in practice is hard to tell.

I just can't see that 2^15 would not be enough for Index.

For PackIndex, 2^16 seems reasonable to me, but in the worst case someone complains, we can just store a larger PackIndex in a tail allocated field, while we keep storing small ones in the TypeBitfields.

================
Comment at: clang/include/clang/AST/Type.h:1838
     /// metaprogramming we'd prefer to keep it as large as possible.
-    /// At the moment it has been left as a non-bitfield since this type
-    /// safely fits in 64 bits as an unsigned, so there is no reason to
-    /// introduce the performance impact of a bitfield.
-    unsigned NumArgs;
+    unsigned NumArgs : 16;
   };
----------------
erichkeane wrote:
> mizvekov wrote:
> > davrec wrote:
> > > I can't imagine that limiting template arg index to 16 bits from 32 could be all that limiting, but given the comment in the original have you tested/confirmed that this is acceptable?
> > Not yet, we will begin performance testing soon. But I am not concerned about this one, as it's easy to get around this limitation by not using the bitfields.
> Did the testing here result in finding anyone who used this?  I'm sure libcxx or some of the boost libraries do a lot of MP on large sizes (though I note libcxx seems to have passed pre-commit).
Not on anything LLVM pre-commit as you saw.

I am not sure how much we should be concerned about here.
This is still much larger than implimits. If this hits anyone, we will just need to add a test case for a reasonable limit and extend this node to hold large values in tail-allocated extra fields.

================
Comment at: clang/include/clang/Sema/Template.h:80
+    struct ArgumentListLevel {
+      Decl *AssociatedDecl;
+      ArgList Args;
----------------
davrec wrote:
> mizvekov wrote:
> > davrec wrote:
> > > Actually I think this one should be changed back to `ReplacedDecl` :)
> > > ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make better sense in STTPT.
> > I would be against introducing another term to refer to the same thing...
> The reason we need this unfortunately vague term "AssociatedDecl" in STTPT is because it can either be a template/template-like declaration *or* a TemplateTypeParmDecl.  But here in MLTAL, it won't be a TTPD, will it?  It will always be the parent template/template-like declaration, right?  So there is no need for vagueness.  `ReplacedDecl` or `ParentTemplate` or something like that seems more appropriate.  
No, it can be the TTPD which is used to represent the invented template for a requires substitution.

================
Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > mizvekov wrote:
> > > ChuanqiXu wrote:
> > > > mizvekov wrote:
> > > > > ChuanqiXu wrote:
> > > > > > mizvekov wrote:
> > > > > > > ChuanqiXu wrote:
> > > > > > > > ChuanqiXu wrote:
> > > > > > > > > ChuanqiXu wrote:
> > > > > > > > > > mizvekov wrote:
> > > > > > > > > > > ChuanqiXu wrote:
> > > > > > > > > > > > I still don't get the reason for the move. What's the benefit? Or why is it necessary?
> > > > > > > > > > > Yeah, now the type can reference the template decl, so without moving this, it can happen during import of the type that we try to read this function template bits without having imported them yet.
> > > > > > > > > > Oh, I guess I met the problem before (D129748 ) and I made a workaround for it (https://reviews.llvm.org/D130331). If I understood right, the patch will solve that problem. I'll check it out later.
> > > > > > > > > > 
> > > > > > > > > > (This kind of code move looks dangerous you know and I'll take a double check)
> > > > > > > > > After looking into the detailed change for the serialization part, I feel it is a not-so-good workaround indeed.. It looks like we need a better method to delay reading the type in the serializer. And I'm looking at it. @mizvekov would you like to rebase the series of patches to the main branch so that I can test it actually.
> > > > > > > > Or would it be simpler to rebase and squash them into a draft revision?
> > > > > > > I had given this some thought, and it made sense to me that we should deal with the template bits first, since these are closer to the introducer for these declarations, and so that it would be harder to have a dependence the other way around.
> > > > > > > 
> > > > > > > But I would like to hear your thoughts on this after you have taken a better look.
> > > > > > > I am working on a bunch of things right now, I should be able to rebase this on the next few days, but otherwise
> > > > > > > I last rebased about 4 days ago, so you can also check that out at https://github.com/mizvekov/llvm-project/tree/resugar
> > > > > > > That link has the whole stack, you probably should check out just the commit for this patch, as you are probably going to encounter issues with the resugarer if you try it on substantial code bases.
> > > > > > > It will carry other changes with it, but I think those should be safe.
> > > > > > I won't say it is bad to deal with template bits first. I just feel it is a workaround to avoid the circular dependent problem in deserialization. Or in another word, here the method works due to you put some decls* in the template parameter types. And we avoid the circular dependent problem by adjusting the order we deserializes. The reasons why I don't feel it is good include:
> > > > > > (1) Although we touched template function decl and template var decl, we don't touched template var decl. I guess it'll be a problem now or later.
> > > > > > (2) The solution here can't solve the similar circular dependent problem I sawed in attributes. So the method only workarounds some resulting of the same problem.
> > > > > > 
> > > > > > Or in one shorter explanation, it should be greater to solve the root problems. I have an idea and I am going to to do a proof-of-concept implementation first since I feel like nobody are happy about an unimplementable idea. Generally I don't like to block patches due to such reasons since it is completely not your fault but I guess it may be better to wait some time. Since if we want to allow workarounds first and clear the workarounds, things will be harder. If you want a timeline, I guess 2 months may be reasonable choices. I mean if I can't make it in 2 months and other reviewers feel this is good (what I am seeing), I feel bad to block this. (But if we're more patient, it'll be better). How do you think about this?
> > > > > Well we touch FunctionTemplates and VariableTemplates in this patch, because they were not doing template first.
> > > > > For whatever reason, class templates were already doing template first, so no need to fix that.
> > > > > 
> > > > > So this patch at least puts that into consistency.
> > > > > 
> > > > > Also, this patch is a pre-requisite for the template resugaring specialization project I am working on, and the deadline for the whole project is about two months from now.
> > > > > 
> > > > > If I leave merging this patch until the end, it seems impossible that I will finish in time, as we will leave field testing this to the very end.
> > > > > 
> > > > > So while I understand the need for a better approach, it is indeed putting me in an impossible situation.
> > > > > Also, this patch is a pre-requisite for the template resugaring specialization project I am working on, and the deadline for the whole project is about two months from now.
> > > > 
> > > > What is the deadline you're referring? According to https://llvm.org/docs/HowToReleaseLLVM.html, the next release branch will be in January. 
> > > > 
> > > > > So while I understand the need for a better approach, it is indeed putting me in an impossible situation.
> > > > 
> > > > I see. I understand it is bad to make perfect the enemy of better. I'll try to give a faster response.
> > > > What is the deadline you're referring? According to https://llvm.org/docs/HowToReleaseLLVM.html, the next release branch will be in January.
> > > 
> > > This is a GSoC that is fast becoming a GWoC, since it has been extended to the maximum possible amount of time already.
> > > 
> > > 
> > I see. Although GSoC projects are not guaranteed to be landed, I don't want to block/object this.
> Update: when I took a look at this again. I found it break a my toy implementation for std modules (https://github.com/ChuanqiXu9/stdmodules). I reduced the failure and submit it directly at here: https://github.com/llvm/llvm-project/commit/1aaba40dcbe8fdc93d825d1f4e22edaa3e9aa5b1 since the more testing should be always good. I guess the reason may be that when we read the function decl, we need to defer reading its type. But I had no time to check. I am going to take vacation in the next 2 weeks so probably I can't respond quickly. 
This one was actually because merging of FunctionTemplateDecls was a bit more complicated, I made changes so that we always merge them from the FunctionDecl side, and it's working now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858