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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
