[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#4470358 , @iains wrote: > In D126694#4470297 , @ChuanqiXu > wrote: > >>> Yes, that was the decision at the last time we looked - because removing >>> decls would degrade

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. In D126694#4470297 , @ChuanqiXu wrote: >> Yes, that was the decision at the last time we looked - because removing >> decls would degrade this - if we have new information that changes our >> preferred design, then fine. > > I

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. > Yes, that was the decision at the last time we looked - because removing > decls would degrade this - if we have new information that changes our > preferred design, then fine. I remember the major reason for the last time to not remove the decls are that the

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. In D126694#4470261 , @ChuanqiXu wrote: >> That is clearly a big motivation - I will ask the folks we were talking to >> at WG21 if that is their priority - or maybe they care about language >> isolation etc. > > Yeah, I know the

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. > That is clearly a big motivation - I will ask the folks we were talking to at > WG21 if that is their priority - or maybe they care about language isolation > etc. Yeah, I know the folks in WG21 prefer the language isolation. But you know, there are many folks who

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. In D126694#4470250 , @ChuanqiXu wrote: > BTW, in my experience for talking about modules to users, they mainly/mostly > care about the compilation performance. And I can't image how many people > would like to use modules if they

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. BTW, in my experience for talking about modules to users, they mainly/mostly care about the compilation performance. And I can't image how many people would like to use modules if they know they won't get a compilation performance win. Repository: rG LLVM Github

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#4470235 , @iains wrote: > In D126694#4470139 , @ChuanqiXu > wrote: > >> Now I think the feature may be important for the performance of modules. And >> I feel we should

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. In D126694#4470139 , @ChuanqiXu wrote: > Now I think the feature may be important for the performance of modules. And > I feel we should work on the ASTWriter side instead of ASTReader side. Since > the size of BMIs is a problem

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. Now I think the feature may be important for the performance of modules. And I feel we should work on the ASTWriter side instead of ASTReader side. Since the size of BMIs is a problem now also it shows that it is not free to load the large BMIs. So while it is

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. I haven't looked into the details. But I feel the feature worth a sentence in ReleaseNotes. (Maybe in the potential-breaking-change section). Also I think it'll be better to have an option `-fenable-discarding-unreachable-decls-in-gmf` to control the behavior. It'll

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 462657. iains added a comment. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. rebased and reworked. The version here has now been tested to consume all of the libc++ headers including those in experimental and ext. Repository: rG

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 447094. iains added a comment. rebase, update testcases for upstream changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126694/new/ https://reviews.llvm.org/D126694 Files:

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3638069 , @iains wrote: > In D126694#3637690 , @ChuanqiXu > wrote: > >> In D126694#3635207 , @iains wrote: >> >>> @rsmith,

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. In D126694#3637690 , @ChuanqiXu wrote: > In D126694#3635207 , @iains wrote: > >> @rsmith, @ChuanqiXu apologies for the multiple revisions, this has turned >> out to be much more involved

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3635207 , @iains wrote: > @rsmith, @ChuanqiXu apologies for the multiple revisions, this has turned out > to be much more involved than I imagined from the standard's text. > > In D126694#3629254

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. @rsmith, @ChuanqiXu apologies for the multiple revisions, this has turned out to be much more involved than I imagined from the standard's text. In D126694#3629254 , @ChuanqiXu wrote: > In D126694#3629251

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 442849. iains marked 25 inline comments as done. iains added a comment. rebased, addressed review comments, added another test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126694/new/

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3629251 , @iains wrote: > In D126694#3629094 , @ChuanqiXu > wrote: > >> BTW, after I applied the patch, the compiler crashes at >> https://github.com/ChuanqiXu9/stdmodules.

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added a comment. In D126694#3629094 , @ChuanqiXu wrote: > BTW, after I applied the patch, the compiler crashes at > https://github.com/ChuanqiXu9/stdmodules. That link points to a project - is there (say) a gist of the crash information? > I

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. BTW, after I applied the patch, the compiler crashes at https://github.com/ChuanqiXu9/stdmodules. I would try to add more tests about C++20 Modules. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126694/new/

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

rsmith added inline comments. Comment at: clang/include/clang/Sema/Sema.h:2275-2276 + llvm::SmallPtrSet SeenDecls; + llvm::SmallPtrSet SeenTypes; + These names seem too general to live directly in `Sema`. Comment at:

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:624 bool isModulePrivate() const { return getModuleOwnershipKind() == ModuleOwnershipKind::ModulePrivate; } ChuanqiXu wrote: > According to the opinion from @rsmith, the

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 440915. iains marked an inline comment as done. iains added a comment. rebased after D113545 was landed and removed that as a parent. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:624 bool isModulePrivate() const { return getModuleOwnershipKind() == ModuleOwnershipKind::ModulePrivate; } According to the opinion from @rsmith, the discarded

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 438968. iains marked 2 inline comments as done. iains added a comment. reabsed onto latest version of D113545 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126694/new/

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3577815 , @iains wrote: > In D126694#3576853 , @ChuanqiXu > wrote: > >> In D126694#3576602 , @iains wrote: >> >>> @ChuanqiXu - I

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains marked 3 inline comments as done. iains added a comment. In D126694#3576853 , @ChuanqiXu wrote: > In D126694#3576602 , @iains wrote: > >> @ChuanqiXu - I changed the module ownership name to

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 436378. iains marked 2 inline comments as done. iains added a comment. rebased, fixed the fail for explicitly-specialized-template.cpp. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126694/new/

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3576602 , @iains wrote: > @ChuanqiXu - I changed the module ownership name to "ModuleDiscardable" - > because, while we are permitted to discard these, we might choose not to (to > give your better diagnostics) -

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains marked 3 inline comments as done. iains added a comment. @ChuanqiXu - I changed the module ownership name to "ModuleDiscardable" - because, while we are permitted to discard these, we might choose not to (to give your better diagnostics) - but we still need to treat them as non-reachable

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

iains updated this revision to Diff 436245. iains edited the summary of this revision. iains added a comment. this is a rework of the implementation. it now passes all test-cases except one - which is one of the cases added by D113545 ; that test-case now seems