[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > Except that it will make using the stl and related modules _much_ faster > which is a significant quality of life improvement imo. > > EDIT: And make the clang peak memory when using modules lower :) Of course, this is why we want this. But for releases, I don't think we should take the risk for the actually NFC change. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: Except that it will make using the stl and related modules *much* faster which is a significant quality of life improvement imo. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > > Great! In that case, I would like to proceed with landing the commits > > individually so they can be more easily reverted if needed (see discussion > > at the beginning of the PR), and then we can see if we can / want to > > backport for `release/22.x`. Please object if you see a problem with that > > plan. > > As this patch doesn't add any new functionalities. I think we don't need to > backport it. No, it doesn't add new functionalities but it finally fixes the feature of lazy template loading to actually work. But backporting is not a requirement, we can continue to carry the patches downstream in ROOT. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > > Great! In that case, I would like to proceed with landing the commits > > > individually so they can be more easily reverted if needed (see > > > discussion at the beginning of the PR), and then we can see if we can / > > > want to backport for `release/22.x`. Please object if you see a problem > > > with that plan. > > > > > > As this patch doesn't add any new functionalities. I think we don't need to > > backport it. > > No, it doesn't add new functionalities but it finally fixes the feature of > lazy template loading to actually work. But backporting is not a requirement, > we can continue to carry the patches downstream in ROOT. hmmm for functionalities, I mean something the end user can notice. The lazy template loading is not a functionality in my mind too. It (including this patch) is an optimization. But this doesn't matter : ) https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > Thanks for waiting! I ran a build of most of our code with this PR and > > didn't find any issues. > > Great! In that case, I would like to proceed with landing the commits > individually so they can be more easily reverted if needed (see discussion at > the beginning of the PR), and then we can see if we can / want to backport > for `release/22.x`. Please object if you see a problem with that plan. As this patch doesn't add any new functionalities. I think we don't need to backport it. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > Thanks for waiting! I ran a build of most of our code with this PR and didn't > find any issues. Great! In that case, I would like to proceed with landing the commits individually so they can be more easily reverted if needed (see discussion at the beginning of the PR), and then we can see if we can / want to backport for `release/22.x`. Please object if you see a problem with that plan. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > @alexfh sorry for the additional ping, I know that the end of the year is > usually vacation time. However, given that `release/22.x` will branch next > week, I wondered if you managed to test and we could still target to finally > get these fixes in? Sorry for the delayed response. I was indeed on vacation, but now I'm back to work and I can test the PR. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
https://github.com/hahnjo ready_for_review https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: @alexfh sorry for the additional ping, I know that the end of the year is usually vacation time. However, given that `release/22.x` will branch next week, I wondered if you managed to test and we could still target to finally get these fixes in? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > If I understand correctly, we could implement this locally in > `ASTReaderDecl.cpp`, matching a number of patterns in > `ASTReader::finishPendingActions()`: > > ```diff > diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp > b/clang/lib/Serialization/ASTReaderDecl.cpp > index 882d54f31280..ec7c2449d77e 100644 > --- a/clang/lib/Serialization/ASTReaderDecl.cpp > +++ b/clang/lib/Serialization/ASTReaderDecl.cpp > @@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData( > auto *Def = DD.Definition; > DD = std::move(MergeDD); > DD.Definition = Def; > -for (auto *D : Def->redecls()) > - cast(D)->DefinitionData = ⅅ > +// FIXME: this duplicates logic from ASTReader::finishPendingActions()... > +for (auto *R = Reader.getMostRecentExistingDecl(Def); R; > + R = R->getPreviousDecl()) > + cast(R)->DefinitionData = ⅅ > return; >} > > ``` https://github.com/llvm/llvm-project/pull/172559 landed this change and fixed the reported crashes (https://github.com/llvm/llvm-project/pull/172559#discussion_r2636632321). @alexfh if you have time, could you give this PR one more run? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > > I'd feel uncomfortable exposing a `noload_redecls` interface because it > > > can potentially break the AST invariants. I think allowing non-modules > > > developers to choose when to complete a redeclaration chain will > > > introduce more bugs than it can fix ;) > > > I'd propose to move forward with `getMostRecentExistingDecl` if it does > > > the job. We can have a separate PR discussing the ergonomics if there is > > > a strong feeling about going this way. > > > > > > I'd like it as: (1) We already have `noload_lookup` and other similar > > "unpublic" API (e.g., `getOwningModuleID` `invalidateCachedLinkage`) in > > DeclBase.h. > > Well, sure, `noload_lookup` is done to check what's the current state before > completing something externally. In fact, I think we should keep these > interfaces at minimum and only use them where we really have no other way out. I feel it sounds over nervous. It is not that bad. And the interfaces will look very consistency. We add the interface doesn't mean we ask programmers to use that. It is simply an interface doing the natural things. > > > (2) Merging Redecls is a major performance hurting point of using modules. > > Only when the modules approach is not bottom up. The non-bottom up approach > has bigger problems than performance at the moment and frankly we cannot > reasonably support it right now... Even with the bottom up approach, we may face the merging problems due to generated decls (e.g, template instantiations) but just mitigates the problem. And for C++20 named modules, if we don't export macros (which a lot of people are not in favor), the user may have to face that problem in practice. For that I sent https://discourse.llvm.org/t/request-for-discussion-clang-a-bidirectional-decl-query-system/88945 and `noload_decls` is necessary for that. > > > (3) In the past, there are many "fix" which just tried to mitigate the bug > > by loading all redecls without understanding the real problem. > > Does that need to happen in this PR? I believe it brings down the peak memory > significantly for most of the module users and we should not wait too long to > get this merged. If the testing results shows the current failure in google side shows it is needed, yes it is needed for the PR. Otherwise, it is not. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > > I'd feel uncomfortable exposing a `noload_redecls` interface because it can > > potentially break the AST invariants. I think allowing non-modules > > developers to choose when to complete a redeclaration chain will introduce > > more bugs than it can fix ;) > > I'd propose to move forward with `getMostRecentExistingDecl` if it does the > > job. We can have a separate PR discussing the ergonomics if there is a > > strong feeling about going this way. > > I'd like it as: (1) We already have `noload_lookup` and other similar > "unpublic" API (e.g., `getOwningModuleID` `invalidateCachedLinkage`) in > DeclBase.h. Well, sure, `noload_lookup` is done to check what's the current state before completing something externally. In fact, I think we should keep these interfaces at minimum and only use them where we really have no other way out. > (2) Merging Redecls is a major performance hurting point of using modules. Only when the modules approach is not bottom up. The non-bottom up approach has bigger problems than performance at the moment and frankly we cannot reasonably support it right now... > (3) In the past, there are many "fix" which just tried to mitigate the bug by > loading all redecls without understanding the real problem. Does that need to happen in this PR? I believe it brings down the peak memory significantly for most of the module users and we should not wait too long to get this merged. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > Sent #170823 > > @alexfh please test this with this PR. I've started testing of https://github.com/llvm/llvm-project/pull/170090 + https://github.com/llvm/llvm-project/pull/170823 + this PR. I'll get back with the results in a couple of days. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > I'd feel uncomfortable exposing a `noload_redecls` interface because it can > potentially break the AST invariants. I think allowing non-modules developers > to choose when to complete a redeclaration chain will introduce more bugs > than it can fix ;) > > I'd propose to move forward with `getMostRecentExistingDecl` if it does the > job. We can have a separate PR discussing the ergonomics if there is a strong > feeling about going this way. I'd like it as: (1) We already have `noload_lookup` and other similar "unpublic" API (e.g., `getOwningModuleID` `invalidateCachedLinkage`) in DeclBase.h. (2) Merging Redecls is a major performance hurting point of using modules. (3) In the past, there are many "fix" which just tried to mitigate the bug by loading all redecls without understanding the real problem. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: Sent https://github.com/llvm/llvm-project/pull/170823 @alexfh please test this with this PR. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > But I don't think that need to be tested by google. It is just an new API. To clarify, what needs testing is the new API plus this PR on top to see if it addresses the problem reported by @alexfh, and if there are others afterwards. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > I don't feel it is hard or you need to fight any thing. > > I invite you to submit a patch. Once that passed review and is confirmed > working with the Google internal code base, I can come back to this PR. I can do it. But I don't think that need to be tested by google. It is just an new API. And the testing process is too slow. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > I don't feel it is hard or you need to fight any thing. I invite you to submit a patch. Once that passed review and is confirmed working with the Google internal code base, I can come back to this PR. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: Let me say it clearly: I will not spend time on it, if I've demonstrated that existing patterns would solve the problem. We have lazy template loading working locally since years, since March with the upstream implementation including these fixes. I think it would be worthwhile to have it fixed upstream because right now it's basically useless, but there's a limit to my involvement. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > Yeah, if you already have this, why do you think it is burden to implement > > `noload_redecls()`? You've already reached it! > > Because it's a local implementation that works in `ASTReader`. I don't want > to think about the more general picture of a function that is usable in all > of Clang (that will need rounds of review), nor fight with C++ iterators. But `getPreviousDecl` may trigger deserialization too. Please look at `getPrevious()` in `Redeclarable::DeclLink::getPrevious`. I don't feel it is hard or you need to fight any thing. We already have `getLatestNotUpdated()` and we can implement `getPreviousNotUpdated()` similarly, then we're almost done. If we're tired of invalid iterations, we can use `make_early_inc_range`. HDYT? The patch have already been stalled by 8 months. I feel all of us are patient enough. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > Yeah, if you already have this, why do you think it is burden to implement > `noload_redecls()`? You've already reached it! Because it's a local implementation that works in `ASTReader`. I don't want to think about the more general picture of a function that is usable in all of Clang (that will need rounds of review), nor fight with C++ iterators. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > I feel they are workarounds. The `noload_redecls()` new interface may be > > the cleanest solution. > > For the first approach, I can potentially agree, even though we are already > deserializing with the current code so it can hardly be worse than that. For > the other two, can you please elaborate why you consider them workarounds? As > I pointed out, they are patterns that are already used in `ASTReader.cpp` and > `ASTReaderDecl.cpp`, for precisely the reasons we are running into now. Maybe we have understanding for workarounds, but that doesn't matter. `noload_redecls` is pretty clear than the patterns you mentioned. > Also my understanding is that `getMostRecentExistingDecl` is implementation > wise exactly what you have in mind for `noload_redecls()`, minus the iterator > interface that I personally don't want to spend effort on. Yeah, if you already have this, why do you think it is burden to implement `noload_redecls()`? You've already reached it! https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > I feel they are workarounds. The `noload_redecls()` new interface may be the > cleanest solution. For the first approach, I can potentially agree, even though we are already deserializing with the current code so it can hardly be worse than that. For the other two, can you please elaborate why you consider them workarounds? As I pointed out, they are patterns that are already used in `ASTReader.cpp` and `ASTReaderDecl.cpp`, for precisely the reasons we are running into now. Also my understanding is that `getMostRecentExistingDecl` is implementation wise exactly what you have in mind for `noload_redecls()`, minus the iterator interface that I personally don't want to spend effort on. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote:
> > As I hinted above, that doesn't work. More specifically, I tried
> > ```diff
> > diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp
> > b/clang/lib/Serialization/ASTReaderDecl.cpp
> > index 882d54f31280..d63824c4ba18 100644
> > --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> > +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> > @@ -2107,8 +2107,9 @@ void ASTDeclMerger::MergeDefinitionData(
> > auto *Def = DD.Definition;
> > DD = std::move(MergeDD);
> > DD.Definition = Def;
> > -for (auto *D : Def->redecls())
> > - cast(D)->DefinitionData = ⅅ
> > +Def = Def->getMostRecentDecl();
> > +while ((Def = Def->getPreviousDecl()))
> > + cast(Def)->DefinitionData = ⅅ
> > return;
> >}
> >
> > ```
>
> I'm stupid, the condition during the first iteration of the `while` loop will
> skip over the most recent declaration 😵 the following works:
>
> ```diff
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData(
> auto *Def = DD.Definition;
> DD = std::move(MergeDD);
> DD.Definition = Def;
> -for (auto *D : Def->redecls())
> - cast(D)->DefinitionData = ⅅ
> +Def = Def->getMostRecentDecl();
> +do {
> + cast(Def)->DefinitionData = ⅅ
> +} while ((Def = Def->getPreviousDecl()));
> return;
>}
>
> ```
>
> > You can't use `getMostRecentDecl `, this will trigger deserializations.
> > Then it is the same with before. You need to implement `noload_redecls()`,
> > possibly in Redeclarable.h
>
> If I understand correctly, we could implement this locally in
> `ASTReaderDecl.cpp`, matching a number of patterns in
> `ASTReader::finishPendingActions()`:
>
> ```diff
> diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp
> b/clang/lib/Serialization/ASTReaderDecl.cpp
> index 882d54f31280..ec7c2449d77e 100644
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData(
> auto *Def = DD.Definition;
> DD = std::move(MergeDD);
> DD.Definition = Def;
> -for (auto *D : Def->redecls())
> - cast(D)->DefinitionData = ⅅ
> +// FIXME: this duplicates logic from ASTReader::finishPendingActions()...
> +for (auto *R = Reader.getMostRecentExistingDecl(Def); R;
> + R = R->getPreviousDecl())
> + cast(R)->DefinitionData = ⅅ
> return;
>}
>
> ```
>
> ... which then of course begs the question why the existing code in
> `ASTReader::finishPendingActions()` doesn't already take care of it?
>
> Another possibility is to use `merged_decls()`:
>
> ```diff
> diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp
> b/clang/lib/Serialization/ASTReaderDecl.cpp
> index 882d54f31280..1112c64eeb02 100644
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,8 @@ void ASTDeclMerger::MergeDefinitionData(
> auto *Def = DD.Definition;
> DD = std::move(MergeDD);
> DD.Definition = Def;
> -for (auto *D : Def->redecls())
> - cast(D)->DefinitionData = ⅅ
> +for (auto *D : merged_redecls(Def))
> + D->DefinitionData = ⅅ
> return;
>}
>
> ```
>
> Which of these alternatives is preferable (assuming one of them actually
> works for @alexfh)?
I feel they are workarounds. The `noload_redecls()` new interface may be the
cleanest solution.
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote:
> As I hinted above, that doesn't work. More specifically, I tried
>
> ```diff
> diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp
> b/clang/lib/Serialization/ASTReaderDecl.cpp
> index 882d54f31280..d63824c4ba18 100644
> --- a/clang/lib/Serialization/ASTReaderDecl.cpp
> +++ b/clang/lib/Serialization/ASTReaderDecl.cpp
> @@ -2107,8 +2107,9 @@ void ASTDeclMerger::MergeDefinitionData(
> auto *Def = DD.Definition;
> DD = std::move(MergeDD);
> DD.Definition = Def;
> -for (auto *D : Def->redecls())
> - cast(D)->DefinitionData = ⅅ
> +Def = Def->getMostRecentDecl();
> +while ((Def = Def->getPreviousDecl()))
> + cast(Def)->DefinitionData = ⅅ
> return;
>}
>
> ```
I'm stupid, the condition during the first iteration of the `while` loop will
skip over the most recent declaration :dizzy_face: the following works:
```diff
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData(
auto *Def = DD.Definition;
DD = std::move(MergeDD);
DD.Definition = Def;
-for (auto *D : Def->redecls())
- cast(D)->DefinitionData = ⅅ
+Def = Def->getMostRecentDecl();
+do {
+ cast(Def)->DefinitionData = ⅅ
+} while ((Def = Def->getPreviousDecl()));
return;
}
```
> You can't use `getMostRecentDecl `, this will trigger deserializations. Then
> it is the same with before. You need to implement `noload_redecls()`,
> possibly in Redeclarable.h
If I understand correctly, we could implement this locally in
`ASTReaderDecl.cpp`, matching a number of patterns in
`ASTReader::finishPendingActions()`:
```diff
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp
b/clang/lib/Serialization/ASTReaderDecl.cpp
index 882d54f31280..ec7c2449d77e 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2107,8 +2107,10 @@ void ASTDeclMerger::MergeDefinitionData(
auto *Def = DD.Definition;
DD = std::move(MergeDD);
DD.Definition = Def;
-for (auto *D : Def->redecls())
- cast(D)->DefinitionData = ⅅ
+// FIXME: this duplicates logic from ASTReader::finishPendingActions()...
+for (auto *R = Reader.getMostRecentExistingDecl(Def); R;
+ R = R->getPreviousDecl())
+ cast(R)->DefinitionData = ⅅ
return;
}
```
... which then of course begs the question why the existing code in
`ASTReader::finishPendingActions()` doesn't already take care of it?
Another possibility is to use `merged_decls()`:
```diff
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp
b/clang/lib/Serialization/ASTReaderDecl.cpp
index 882d54f31280..1112c64eeb02 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2107,8 +2107,8 @@ void ASTDeclMerger::MergeDefinitionData(
auto *Def = DD.Definition;
DD = std::move(MergeDD);
DD.Definition = Def;
-for (auto *D : Def->redecls())
- cast(D)->DefinitionData = ⅅ
+for (auto *D : merged_redecls(Def))
+ D->DefinitionData = ⅅ
return;
}
```
Which of these alternatives is preferable (assuming one of them actually works
for @alexfh)?
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > > Unfortunately, I don't have an easy idea to fix it - going back to the > > > old code and just calling `getMostRecentDecl` at the beginning doesn't > > > pass the `Modules/GH170084.cpp` test case... > > > > > > I feel it may not be too hard to implement a `noload_redecls()`. You can > > get the most recent decl without calling `getMostRecentDecl ()` (that will > > trigger the deserialization of all redecls, I hate it) in Redeclarable.h > > internally. And you can simply traverse the redecls. > > As I hinted above, that doesn't work. More specifically, I tried > > ```diff > diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp > b/clang/lib/Serialization/ASTReaderDecl.cpp > index 882d54f31280..d63824c4ba18 100644 > --- a/clang/lib/Serialization/ASTReaderDecl.cpp > +++ b/clang/lib/Serialization/ASTReaderDecl.cpp > @@ -2107,8 +2107,9 @@ void ASTDeclMerger::MergeDefinitionData( > auto *Def = DD.Definition; > DD = std::move(MergeDD); > DD.Definition = Def; > -for (auto *D : Def->redecls()) > - cast(D)->DefinitionData = ⅅ > +Def = Def->getMostRecentDecl(); > +while ((Def = Def->getPreviousDecl())) > + cast(Def)->DefinitionData = ⅅ > return; >} > > ``` You can't use `getMostRecentDecl `, this will trigger deserializations. Then it is the same with before. You need to implement `noload_redecls()`, possibly in Redeclarable.h https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > > Unfortunately, I don't have an easy idea to fix it - going back to the old > > code and just calling `getMostRecentDecl` at the beginning doesn't pass the > > `Modules/GH170084.cpp` test case... > > I feel it may not be too hard to implement a `noload_redecls()`. You can get > the most recent decl without calling `getMostRecentDecl ()` (that will > trigger the deserialization of all redecls, I hate it) in Redeclarable.h > internally. And you can simply traverse the redecls. As I hinted above, that doesn't work. More specifically, I tried ```diff diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 882d54f31280..d63824c4ba18 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -2107,8 +2107,9 @@ void ASTDeclMerger::MergeDefinitionData( auto *Def = DD.Definition; DD = std::move(MergeDD); DD.Definition = Def; -for (auto *D : Def->redecls()) - cast(D)->DefinitionData = ⅅ +Def = Def->getMostRecentDecl(); +while ((Def = Def->getPreviousDecl())) + cast(Def)->DefinitionData = ⅅ return; } ``` https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > With both PRs I'm getting at least this Clang assertion error on some of > > our code. There's more, and I'll try to create a reproducer for this one, > > but maybe the stack trace is enough to figure out what's wrong: > > ``` > > assert.h assertion failed at clang/include/clang/AST/Redeclarable.h:262 in > > redecl_iterator > > &clang::Redeclarable::redecl_iterator::operator++() [de > > cl_type = clang::TagDecl]: 0 && "Passed first decl twice, invalid redecl > > chain!" > > @ 0x561251408044 __assert_fail > > @ 0x56124c214b91 clang::ASTDeclMerger::MergeDefinitionData() > > @ 0x56124c21722e > > clang::ASTDeclReader::VisitClassTemplateSpecializationDeclImpl() > > @ 0x56124c207953 clang::declvisitor::Base<>::Visit() > > @ 0x56124c207135 clang::ASTDeclReader::Visit() > > @ 0x56124d69ddcf > > clang::StackExhaustionHandler::runWithSufficientStackSpace() > > ``` > > Thanks for testing! "Luckily" it's the exact code that I touched in > `MergeDefinitionData` in #170090. I guess it comes down to @ChuanqiXu9's > comment in [#170090 > (comment)](https://github.com/llvm/llvm-project/pull/170090#discussion_r2576320393) > that `redecls()` can trigger deserialization and that may invalidate the > iterator (?). Unfortunately, I don't have an easy idea to fix it - going back > to the old code and just calling `getMostRecentDecl` at the beginning doesn't > pass the `Modules/GH170084.cpp` test case... I feel it may not be too hard to implement a `noload_redecls()`. You can get the most recent decl without calling `getMostRecentDecl ()` (that will trigger the deserialization of all redecls, I hate it) in Redeclarable.h internally. And you can simply traverse the redecls. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > With both PRs I'm getting at least this Clang assertion error on some of our > code. There's more, and I'll try to create a reproducer for this one, but > maybe the stack trace is enough to figure out what's wrong: > > ``` > assert.h assertion failed at clang/include/clang/AST/Redeclarable.h:262 in > redecl_iterator > &clang::Redeclarable::redecl_iterator::operator++() [de > cl_type = clang::TagDecl]: 0 && "Passed first decl twice, invalid redecl > chain!" > @ 0x561251408044 __assert_fail > @ 0x56124c214b91 clang::ASTDeclMerger::MergeDefinitionData() > @ 0x56124c21722e > clang::ASTDeclReader::VisitClassTemplateSpecializationDeclImpl() > @ 0x56124c207953 clang::declvisitor::Base<>::Visit() > @ 0x56124c207135 clang::ASTDeclReader::Visit() > @ 0x56124d69ddcf > clang::StackExhaustionHandler::runWithSufficientStackSpace() > ``` Thanks for testing! "Luckily" it's the exact code that I touched in `MergeDefinitionData` in https://github.com/llvm/llvm-project/pull/170090. I guess it comes down to @ChuanqiXu9's comment in https://github.com/llvm/llvm-project/pull/170090#discussion_r2576320393 that `redecls()` can trigger deserialization and that may invalidate the iterator (?). Unfortunately, I don't have an easy idea to fix it - going back to the old code and just calling `getMostRecentDecl` at the beginning doesn't pass the `Modules/GH170084.cpp` test case... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > Ok, this wasn't too bad actually: #170090 fixes the reproducer, so fingers > crossed it also addresses the issue in the full-blown internal code 🤞 @alexfh > if you have more cycles available, could you test the two PRs together? With both PRs I'm getting at least this Clang assertion error on some of our code. There's more, and I'll try to create a reproducer for this one, but maybe the stack trace is enough to figure out what's wrong: ``` assert.h assertion failed at clang/include/clang/AST/Redeclarable.h:262 in redecl_iterator &clang::Redeclarable::redecl_iterator::operator++() [de cl_type = clang::TagDecl]: 0 && "Passed first decl twice, invalid redecl chain!" @ 0x561251408044 __assert_fail @ 0x56124c214b91 clang::ASTDeclMerger::MergeDefinitionData() @ 0x56124c21722e clang::ASTDeclReader::VisitClassTemplateSpecializationDeclImpl() @ 0x56124c207953 clang::declvisitor::Base<>::Visit() @ 0x56124c207135 clang::ASTDeclReader::Visit() @ 0x56124d69ddcf clang::StackExhaustionHandler::runWithSufficientStackSpace() @ 0x56124c22e7f1 clang::ASTReader::ReadDeclRecord() @ 0x56124c1aa595 clang::ASTReader::GetDecl() @ 0x56124c237639 clang::ASTReader::ReadDeclAs<>() @ 0x56124c20800b clang::ASTDeclReader::VisitDecl() @ 0x56124c209e52 clang::ASTDeclReader::VisitValueDecl() @ 0x56124c20a175 clang::ASTDeclReader::VisitDeclaratorDecl() @ 0x56124c20ae6a clang::ASTDeclReader::VisitFunctionDecl() @ 0x56124c2152b6 clang::ASTDeclReader::VisitCXXMethodDecl() @ 0x56124c207135 clang::ASTDeclReader::Visit() @ 0x56124d69ddcf clang::StackExhaustionHandler::runWithSufficientStackSpace() @ 0x56124c22e7f1 clang::ASTReader::ReadDeclRecord() @ 0x56124c1aa595 clang::ASTReader::GetDecl() @ 0x56124c24af78 clang::ASTStmtReader::VisitMemberExpr() @ 0x56124c25b2c1 clang::ASTReader::ReadStmtFromStream() @ 0x56124c1b585d clang::ASTReader::GetExternalDeclStmt() @ 0x56124d1f8839 clang::FunctionDecl::getBody() @ 0x56124d2053f1 clang::FunctionDecl::getBody() @ 0x56124b52493d clang::CodeGen::CodeGenFunction::GenerateCode() @ 0x56124b55084c clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition() @ 0x56124b547ea2 clang::CodeGen::CodeGenModule::EmitGlobalDefinition() @ 0x56124b538856 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b538872 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x56124b5351ee clang::CodeGen::CodeGenModule::Release() @ 0x56124b67acee (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit() @ 0x56124b1d3b9a clang::BackendConsumer::HandleTranslationUnit() @ 0x56124c077788 clang::ParseAST() @ 0x56124bdad5aa clang::FrontendAction::Execute() @ 0x56124bd2145d clang::CompilerInstance::ExecuteAction() @ 0x56124b1d316e clang::ExecuteCompilerInvocation() @ 0x56124b1c65ea cc1_main() @ 0x56124b1c3619 ExecuteCC1Tool() @ 0x56124b1c5dcc llvm::function_ref<>::callback_fn<>() @ 0x56124bee291e llvm::function_ref<>::callback_fn<>() @ 0x56125108241c llvm::CrashRecoveryContext::RunSafely() @ 0x56124bee1e04 clang::driver::CC1Command::Execute() @ 0x56124be9f453 clang::driver::Compilation::ExecuteCommand() @ 0x56124be9f6df clang::driver::Compilation::ExecuteJobs() @ 0x56124beb9dc0 clang::driver::Driver::ExecuteCompilation() @ 0x56124b1c2c1e clang_main() @ 0x56124b1c0f74 main ``` https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: Ok, this wasn't too bad actually: https://github.com/llvm/llvm-project/pull/170090 fixes the reproducer, so fingers crossed it also addresses the issue in the full-blown internal code :crossed_fingers: @alexfh if you have more cycles available, could you test the two PRs together? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > After manually cleaning up the ~10KB that CVise could squeeze out of the > initial 400+MB, I got this: Thanks for your efforts, very useful! That reduced example actually crashes on `main` already, I opened https://github.com/llvm/llvm-project/issues/170084 (after some more simplifications). If your full internal code is currently compiling without problems, it is likely that this PR "just" exposes the issue which git-bisect says was introduced by 91cdd35008e9ab32dffb7e401cdd7313b3461892 on August 9. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote:
After manually cleaning up the ~10KB that CVise could squeeze out of the
initial 400+MB, I got this:
```
//--- a.cppmap
module "a" {
header "a.h"
}
//--- b.cppmap
module "b" {
header "b.h"
}
//--- d.cppmap
module "d" {
header "d.h"
}
//--- stl.cppmap
module "stl" {
header "stl.h"
}
//--- a.h
#include "b.h"
namespace {
void a(absl::set c) {
absl::map a;
absl::set b;
c.end();
c.contains();
}
} // namespace
//--- b.h
#include "c.h"
void b() { absl::set x; }
//--- c.h
#include "stl.h"
inline namespace internal {
template
class set_base {
public:
struct iterator {
void u() const;
};
iterator end() const { return {}; }
void contains() const { end().u(); }
pair e();
template
friend H h();
};
template
class map_base {
template
friend H h();
};
} // namespace internal
namespace absl {
template
class set : public set_base {};
template
class map : public map_base {};
} // namespace absl
//--- d.h
#include "c.h"
void d() { absl::set x; }
//--- stl.h
#ifndef _STL_H_
#define _STL_H_
template
struct pair;
#endif
//--- main.cc
#include "c.h"
void f(absl::set o) { o.contains(); }
```
```
$ $CLANG -fmodule-name=stl -fmodule-map-file=stl.cppmap
-Xclang=-fno-cxx-modules -xc++ -Xclang=-emit-module -fmodules -std=c++20 -c
stl.cppmap -o stl.pcm
$ $CLANG -fmodule-name=d -fmodule-map-file=d.cppmap -Xclang=-fno-cxx-modules
-xc++ -Xclang=-emit-module -fmodules -fmodule-file=stl.pcm -std=c++20 -c
d.cppmap -o d.pcm
$ $CLANG -fmodule-name=b -fmodule-map-file=b.cppmap -Xclang=-fno-cxx-modules
-xc++ -Xclang=-emit-module -fmodules -fmodule-file=stl.pcm -std=c++20 -c
b.cppmap -o b.pcm
$ $CLANG -fmodule-name=a -fmodule-map-file=a.cppmap -Xclang=-fno-cxx-modules
-xc++ -Xclang=-emit-module -fmodules -fmodule-file=stl.pcm -fmodule-file=b.pcm
-fmodule-file=d.pcm -std=c++20 -c a.cppmap -o a.pcm
$ $CLANG -Xclang=-fno-cxx-modules -fmodules -fmodule-file=a.pcm -std=c++20 -c
main.cc -o main.o
assert.h assertion failed at clang/lib/AST/RecordLayoutBuilder.cpp:3377 in
const ASTRecordLayout &clang::ASTContext::getASTRecordLayout(const RecordDecl
*) const: D && "Cannot get layout of forward declarations!"
*** Check failure stack trace: ***
@ 0x55e64e1aed24 __assert_fail
@ 0x55e64a2c7646 clang::ASTContext::getASTRecordLayout()
@ 0x55e649c95e48 clang::ASTContext::getTypeInfoImpl()
@ 0x55e649c97297 clang::ASTContext::getTypeInfo()
@ 0x55e649c979bc clang::ASTContext::getTypeAlignInChars()
@ 0x55e6481188d3 clang::CodeGen::CodeGenFunction::CreateIRTemp()
@ 0x55e6483843bc clang::CodeGen::CodeGenFunction::StartFunction()
@ 0x55e648386716 clang::CodeGen::CodeGenFunction::GenerateCode()
@ 0x55e6483b20ec
clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition()
@ 0x55e6483a9662 clang::CodeGen::CodeGenModule::EmitGlobalDefinition()
@ 0x55e64839a226 clang::CodeGen::CodeGenModule::EmitDeferred()
@ 0x55e64839a242 clang::CodeGen::CodeGenModule::EmitDeferred()
@ 0x55e648396c6e clang::CodeGen::CodeGenModule::Release()
@ 0x55e6484dc8ae (anonymous
namespace)::CodeGeneratorImpl::HandleTranslationUnit()
...
```
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > There's also a number of compilations that became significantly slower (or > hit an infinite loop in Clang). I will try to find time to get to this part too. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > I've done some manual build graph cleanup and got the makefile in the test > case from a few thousand down to a handful of rules. The reduction started > running a bit faster since then: the size of inputs is now ~65MB spread > across ~4.5K files. Looking at how it goes, I may have a shareable test case > by the end of the week. 10MB and 2.7K files https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote:
> I could allocate some time to debugging this, and here's what I got.
Thanks for trying all that!
> It looks like it happens as you suspected ("`D->hasExternalLexicalStorage()`
> returns `false` and we skip the call to `CompleteType`").
Ok, at least the understanding of the problem is progressing.
> I tried changing that condition to `getExternalSource() &&
> !D->getDefinition()`, but it doesn't help - I'm still getting the same
> assertion:
Too bad, that would have been too easy I guess... Now I wonder if we maybe need
to change the order of operations, ie first complete the redecl chain, then
notice that we can get the definition externally, and then actually go and do
that. Conceptually something like
```c++
// Complete the redecl chain (if necessary).
D = D->getMostRecentDecl();
if (D->hasExternalLexicalStorage() && !D->getDefinition())
getExternalSource()->CompleteType(const_cast(D));
```
Not sure if that changes anything (at least it doesn't fail Clang tests when
applied to current `master` on its own), but as I said earlier "redecl chains
are a bit black magic for me". But if we are getting close to getting a
sharable reproducer, I will be able to have a look myself.
> Please let me know, if you have any more ideas I could try.
Only if you have more time to spare, the second part of point 4. above, ie
which of the commits is the culprit: Is it a single one and everything is fine
if you just exclude / revert that one?
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > > Ok, if possible maybe you can keep it running in parallel, we will still > > need a reproducer as a regression test later on. > > The test case is down to ~130MB and 9K files, but still nothing I can share > directly. I've done some manual build graph cleanup and got the makefile in the test case from a few thousand down to a handful of rules. The reduction started running a bit faster since then: the size of inputs is now ~65MB spread across ~4.5K files. Looking at how it goes, I may have a shareable test case by the end of the week. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > > It's all internal code, so sharing it in any way wouldn't be possible. But > > if you have any ideas of how to stuff Clang with debug logs and/or > > assertions to make the issue easier to diagnose, I could run Clang and send > > you logs. > > Not really without further understanding what is going wrong under which > circumstances... Some pointers (not sure how much time you would be able to > spend on your side debugging the problem): > > 1. As a first matter of action, `ASTContext::getASTRecordLayout` completes > the type. This should trigger template loading which is more "lazy" after > these fixes. > > https://github.com/llvm/llvm-project/blob/bba40ab4bd60e636e77362c46c181eafd377f541/clang/lib/AST/RecordLayoutBuilder.cpp#L3371-L3379 > 2. One thing that could happen is that deserialization triggers merging of > declarations and another one has the actual definition attached (redecl > chains are a bit black magic for me). > 3. Another possibility is that the forward declaration is local and not > external, thus `D->hasExternalLexicalStorage()` returns `false` and we skip > the call to `CompleteType`. We might try changing that condition to > `getExternalSource() && !D->getDefinition()`... > 4. Finally it would be interesting to know if what type of template class (?) > we are dealing with: implicit specialization, explicit, involving partial > specializations? In the past, the "triggering" commit was > [3a272ba](https://github.com/llvm/llvm-project/commit/3a272ba3e512218976660912ef9354ff5576c95d) > if you can maybe see if reverting that one avoids the problems (or in > general which of the four changes does)? I could allocate some time to debugging this, and here's what I got. The crash stack is: ``` ... frame #7: 0x748dba8a clang-dbg`__assert_fail(assertion="D && \"Cannot get layout of forward declarations!\"", file="llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp", line=3377, function="const ASTRecordLayout &clang::ASTContext::getASTRecordLayout(const RecordDecl *) const") at logging.cc:61:3 frame #8: 0x6a8dd209 clang-dbg`clang::ASTContext::getASTRecordLayout(this=0x30f1bfc2, D=0x) const at RecordLayoutBuilder.cpp:3377:3 * frame #9: 0x6976f180 clang-dbg`clang::ASTContext::getTypeInfoImpl(this=0x30f1bfc2, T=0x30f1ba458cb0) const at ASTContext.cpp:2409:37 frame #10: 0x69770a6c clang-dbg`clang::ASTContext::getTypeInfo(this=0x30f1bfc2, T=0x30f1ba458cb0) const at ASTContext.cpp:1969:17 frame #11: 0x644077f8 clang-dbg`clang::ASTContext::getTypeInfo(this=0x30f1bfc2, T=QualType @ 0x7ffe6dd8) const at ASTContext.h:2645:51 frame #12: 0x64b475c8 clang-dbg`clang::ASTContext::getTypeAlign(this=0x30f1bfc2, T=QualType @ 0x7ffe6e38) const at ASTContext.h:2682:52 frame #13: 0x697717f8 clang-dbg`clang::ASTContext::getTypeAlignInChars(this=0x30f1bfc2, T=QualType @ 0x7ffe6e70) const at ASTContext.cpp:2574:30 frame #14: 0x64483e7c clang-dbg`clang::CodeGen::CodeGenFunction::CreateIRTemp(this=0x7ffeb608, Ty=QualType @ 0x7ffe6ef8, Name=0x7ffe8740) at CGExpr.cpp:183:34 frame #15: 0x64e95a9e clang-dbg`clang::CodeGen::CodeGenFunction::StartFunction(this=0x7ffeb608, GD=GlobalDecl @ 0x7ffea560, RetTy=QualType @ 0x7ffea558, Fn=0x30f1b47866c8, FnInfo=0x30f1b477d980, Args=0x7ffeb3c0, Loc=(ID = 2115544462), StartLoc=(ID = 2115544504)) at CodeGenFunction.cpp:1248:19 frame #16: 0x64e98bf0 clang-dbg`clang::CodeGen::CodeGenFunction::GenerateCode(this=0x7ffeb608, GD=GlobalDecl @ 0x7ffeb470, Fn=0x30f1b47866c8, FnInfo=0x30f1b477d980) at CodeGenFunction.cpp:1558:3 frame #17: 0x64eee07c clang-dbg`clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(this=0x30f1bfd04000, GD=GlobalDecl @ 0x7ffecfa0, GV=0x30f1b47866c8) at CodeGenModule.cpp:6406:26 frame #18: 0x64eddd64 clang-dbg`clang::CodeGen::CodeGenModule::EmitGlobalDefinition(this=0x30f1bfd04000, GD=GlobalDecl @ 0x7ffed150, GV=0x30f1b47866c8) at CodeGenModule.cpp:4479:9 frame #19: 0x64ecb704 clang-dbg`clang::CodeGen::CodeGenModule::EmitDeferred(this=0x30f1bfd04000) at CodeGenModule.cpp:3562:5 frame #20: 0x64ecb774 clang-dbg`clang::CodeGen::CodeGenModule::EmitDeferred(this=0x30f1bfd04000) at CodeGenModule.cpp:3568:7 frame #21: 0x64ecb774 clang-dbg`clang::CodeGen::CodeGenModule::EmitDeferred(this=0x30f1bfd04000) at CodeGenModule.cpp:3568:7 frame #22: 0x64ecb774 clang-dbg`clang::CodeGen::CodeGenModule::EmitDeferred(this=0x30f1bfd04000) at CodeGenModule.cpp:3568:7 frame #23: 0x64ecb774 clang-dbg`clang::CodeGen::CodeGenModule::EmitDeferred(this=0x30f1bfd04000) at CodeGenM
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > > > The reduction has been crawling for 5 days now. The input is down to > > > 275MB, but the progress has slowed down a lot. > > > > > > Ouch... I don't know if it would already be possible to share it (offline > > with me only if needed) or does it still contain Google-internal code? Then > > I could at least have a look with a debugger and see if I can "guess" what > > is happening > > It's all internal code, so sharing it in any way wouldn't be possible. But if > you have any ideas of how to stuff Clang with debug logs and/or assertions to > make the issue easier to diagnose, I could run Clang and send you logs. Not really without further understanding what is going wrong under which circumstances... Some pointers (not sure how much time you would be able to spend on your side debugging the problem): 1. As a first matter of action, `ASTContext::getASTRecordLayout` completes the type. This should trigger template loading which is more "lazy" after these fixes. https://github.com/llvm/llvm-project/blob/bba40ab4bd60e636e77362c46c181eafd377f541/clang/lib/AST/RecordLayoutBuilder.cpp#L3371-L3379 2. One thing that could happen is that deserialization triggers merging of declarations and another one has the actual definition attached (redecl chains are a bit black magic for me). 3. Another possibility is that the forward declaration is local and not external, thus `D->hasExternalLexicalStorage()` returns `false` and we skip the call to `CompleteType`. We might try changing that condition to `getExternalSource() && !D->getDefinition()`... 4. Finally it would be interesting to know if what type of template class (?) we are dealing with: implicit specialization, explicit, involving partial specializations? In the past, the "triggering" commit was 3a272ba3e512218976660912ef9354ff5576c95d if you can maybe see if reverting that one avoids the problems (or in general which of the four changes does)? > Meanwhile, CVise made some progress (but not much). The input is down to > 250MB and ~12.5K files. Ok, if possible maybe you can keep it running in parallel, we will still need a reproducer as a regression test later on. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > > The reduction has been crawling for 5 days now. The input is down to 275MB, > > but the progress has slowed down a lot. > > Ouch... I don't know if it would already be possible to share it (offline > with me only if needed) or does it still contain Google-internal code? Then I > could at least have a look with a debugger and see if I can "guess" what is > happening It's all internal code, so sharing it in any way wouldn't be possible. But if you have any ideas of how to stuff Clang with debug logs and/or assertions to make the issue easier to diagnose, I could run Clang and send you logs. Meanwhile, CVise made some progress (but not much). The input is down to 250MB and ~12.5K files. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > The reduction has been crawling for 5 days now. The input is down to 275MB, > but the progress has slowed down a lot. Ouch... I don't know if it would already be possible to share it (offline with me only if needed) or does it still contain Google-internal code? Then I could at least have a look with a debugger and see if I can "guess" what is happening https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > > So far I found one assertion failure: > > ``` > > assert.h assertion failed at > > llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp:3377 in const > > ASTRecordLayout &clang::ASTContext::getASTRecordLayout(const RecordDecl *) > > const: D && "Cannot get layout of forward declarations!" > > ... > > Trying to produce a test case. > > ``` > > The reduction is running, but it's going to take a lot of time, I'm afraid > (as it usually is with module-related bugs). The initial size of the inputs > is around 450MB. The reduction has been crawling for 5 days now. The input is down to 275MB, but the progress has slowed down a lot. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: > So far I found one assertion failure: > > ``` > assert.h assertion failed at > llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp:3377 in const > ASTRecordLayout &clang::ASTContext::getASTRecordLayout(const RecordDecl *) > const: D && "Cannot get layout of forward declarations!" > ... > Trying to produce a test case. The reduction is running, but it's going to take a lot of time, I'm afraid (as it usually is with module-related bugs). The initial size of the inputs is around 450MB. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: There's also a number of compilations that became significantly slower (or hit an infinite loop in Clang). https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: So far I found one assertion failure: ``` assert.h assertion failed at llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp:3377 in const ASTRecordLayout &clang::ASTContext::getASTRecordLayout(const RecordDecl *) const: D && "Cannot get layout of forward declarations!" *** Check failure stack trace: *** @ 0x55e7b65cf564 absl::log_internal::LogMessage::SendToLog() @ 0x55e7b65cf525 absl::log_internal::LogMessage::Flush() @ 0x55e7b67aed24 __assert_fail @ 0x55e7b28c7646 clang::ASTContext::getASTRecordLayout() @ 0x55e7b2295e48 clang::ASTContext::getTypeInfoImpl() @ 0x55e7b2297297 clang::ASTContext::getTypeInfo() @ 0x55e7b22979bc clang::ASTContext::getTypeAlignInChars() @ 0x55e7b07188d3 clang::CodeGen::CodeGenFunction::CreateIRTemp() @ 0x55e7b09843bc clang::CodeGen::CodeGenFunction::StartFunction() @ 0x55e7b0986716 clang::CodeGen::CodeGenFunction::GenerateCode() @ 0x55e7b09b20ec clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition() @ 0x55e7b09a9662 clang::CodeGen::CodeGenModule::EmitGlobalDefinition() @ 0x55e7b099a226 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x55e7b099a242 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x55e7b099a242 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x55e7b099a242 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x55e7b099a242 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x55e7b099a242 clang::CodeGen::CodeGenModule::EmitDeferred() @ 0x55e7b0996c6e clang::CodeGen::CodeGenModule::Release() @ 0x55e7b0adc8ae (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit() @ 0x55e7b063685a clang::BackendConsumer::HandleTranslationUnit() @ 0x55e7b14daca8 clang::ParseAST() @ 0x55e7b120f68a clang::FrontendAction::Execute() @ 0x55e7b118333d clang::CompilerInstance::ExecuteAction() @ 0x55e7b0635e2e clang::ExecuteCompilerInvocation() @ 0x55e7b0628f5e cc1_main() @ 0x55e7b0625f99 ExecuteCC1Tool() @ 0x55e7b062874c llvm::function_ref<>::callback_fn<>() @ 0x55e7b1344afe llvm::function_ref<>::callback_fn<>() @ 0x55e7b64224bc llvm::CrashRecoveryContext::RunSafely() @ 0x55e7b1343fe4 clang::driver::CC1Command::Execute() @ 0x55e7b13017b3 clang::driver::Compilation::ExecuteCommand() @ 0x55e7b1301a4f clang::driver::Compilation::ExecuteJobs() @ 0x55e7b131bd20 clang::driver::Driver::ExecuteCompilation() @ 0x55e7b06255a7 clang_main() @ 0x55e7b06238f4 main @ 0x7f012f05a352 __libc_start_main @ 0x55e7b062382a _start ``` Trying to produce a test case. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
alexfh wrote: @hahnjo I'll take care of testing this internally at Google. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: I rebased the changes after #154158. Would it be possible to re-test / give information about the observed failures? I understand (public) reproducers take time, but it would be great to get at least something to understand how to trigger... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/133057
>From cae06d118bb3092cf3b45244b089db6476c9bf72 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:27:59 +0100
Subject: [PATCH 1/4] [Serialization] Hash inner template arguments
The code is applied from ODRHash::AddDecl with the reasoning given
in the comment, to reduce collisions. This was particularly visible
with STL types templated on std::pair where its template arguments
were not taken into account.
---
.../lib/Serialization/TemplateArgumentHasher.cpp | 15 +++
1 file changed, 15 insertions(+)
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp
b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index 353e8a2daa925..da2c70ca17b79 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -202,6 +202,21 @@ void TemplateArgumentHasher::AddDecl(const Decl *D) {
}
AddDeclarationName(ND->getDeclName());
+
+ // If this was a specialization we should take into account its template
+ // arguments. This helps to reduce collisions coming when visiting template
+ // specialization types (eg. when processing type template arguments).
+ ArrayRef Args;
+ if (auto *CTSD = dyn_cast(D))
+Args = CTSD->getTemplateArgs().asArray();
+ else if (auto *VTSD = dyn_cast(D))
+Args = VTSD->getTemplateArgs().asArray();
+ else if (auto *FD = dyn_cast(D))
+if (FD->getTemplateSpecializationArgs())
+ Args = FD->getTemplateSpecializationArgs()->asArray();
+
+ for (auto &TA : Args)
+AddTemplateArgument(TA);
}
void TemplateArgumentHasher::AddQualType(QualType T) {
>From c8183e71f406dcf0361b06fa25e7d5dd08fcac7b Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:31:23 +0100
Subject: [PATCH 2/4] [Serialization] Complete only needed partial
specializations
It is unclear (to me) why this needs to be done "for safety", but
this change significantly improves the effectiveness of lazy loading.
---
clang/lib/Serialization/ASTReader.cpp | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 8b3fd41adb465..dc45c3b17ea68 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8084,14 +8084,8 @@ void ASTReader::CompleteRedeclChain(const Decl *D) {
}
}
- if (Template) {
-// For partitial specialization, load all the specializations for safety.
-if (isa(D))
- Template->loadLazySpecializationsImpl();
-else
- Template->loadLazySpecializationsImpl(Args);
- }
+ if (Template)
+Template->loadLazySpecializationsImpl(Args);
}
CXXCtorInitializer **
>From 3a272ba3e512218976660912ef9354ff5576c95d Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:42:20 +0100
Subject: [PATCH 3/4] [Serialization] Load only needed partial specializations
Similar as the last commit, it is unclear why we need to load all
specializations, including non-partial ones, when we have a TPL.
---
clang/lib/AST/DeclTemplate.cpp | 6 --
1 file changed, 6 deletions(-)
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 2f7ae6d6cac63..e76e46499038c 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -369,12 +369,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl(
if (!ExternalSource)
return false;
- // If TPL is not null, it implies that we're loading specializations for
- // partial templates. We need to load all specializations in such cases.
- if (TPL)
-return
ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
- /*OnlyPartial=*/false);
-
return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
Args);
}
>From c44e2b21d1c87cf8ac42c27962390e910ba67b64 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:59:46 +0100
Subject: [PATCH 4/4] [Serialization] Remove bail-out logic in
TemplateArgumentHasher
While it is correct to assign a single fixed hash to all template
arguments, it can reduce the effectiveness of lazy loading and is
not actually needed: we are allowed to ignore parts that cannot be
handled because they will be analogously ignored by all hashings.
---
.../Serialization/TemplateArgumentHasher.cpp | 36 ++-
1 file changed, 3 insertions(+), 33 deletions(-)
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp
b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index da2c70ca17b79..6b598d26c3098 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -22,17 +22,6 @@ using namespace clang;
namespace {
class TemplateArgumentHasher {
- // If
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: @ilya-biryukov @emaxx-google ping, any updates on this? I would be curious to learn more details about the "rather special build mode" and, if possible, if https://github.com/llvm/llvm-project/pull/154158 on its own is good to go https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: Hm, then I need more information how to reproduce before I can meaningfully look into it... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > > Ok, that's progress at least. I will then land the PR with the fix some > > > time soon and we can take that item off the list. > > > > > > How can we make sure the new reported errors are not triggered by #154158? > > #154158 "relaxes" a check so that more conversion functions will compare > equal (and subsequently be disambiguated). Also the fact that @emaxx-google > mentions a "special build mode" (which probably involves > `-fno-skip-odr-check-in-gmf`) makes me suspect that we're dealing with a > different error here. I believe Google was not using `-fno-skip-odr-check-in-gmf`. It is specific to C++20 named modules. #154158 may trigger other loading, so I am not sure they are not relevent. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > > Ok, that's progress at least. I will then land the PR with the fix some > > time soon and we can take that item off the list. > > How can we make sure the new reported errors are not triggered by #154158? https://github.com/llvm/llvm-project/pull/154158 "relaxes" a check so that more conversion functions will compare equal (and subsequently be disambiguated). Also the fact that @emaxx-google mentions a "special build mode" (which probably involves `-fno-skip-odr-check-in-gmf`) makes me suspect that we're dealing with a different error here. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > Yes it seems that the original target in question compiles successfully now. Ok, that's progress at least. I will then land the PR with the fix some time soon and we can take that item off the list. > Also all new errors seem to come from a rather special build mode. Hm, maybe you can share more information already before providing a reproducer, such as special compiler flags used? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > > Invested some time today and I think the fix is #154158. It works for the > > reproducer posted in [#133057 > > (comment)](https://github.com/llvm/llvm-project/pull/133057#issuecomment-2886816972), > > @ilya-biryukov or @emaxx-google if you have some time, would it be > > possible to test the two PRs together in your infrastructure? > > Unfortunately after repeating the testing on the google monorepo with these > two changes cherry-picked, there's a lot of errors observed. Minimizing > examples would take a few days, but to name some error messages: > > * `'std::tuple::operator=' from module '...' is not > present in definition of 'std::tuple' provided earlier` > > * `'std::pair::operator=' from module '...' is > not present in definition of 'std::pair' provided > earlier` Hm, and that's after or instead of the previous problems. Ie did https://github.com/llvm/llvm-project/pull/154158 fix a problem and we get past that point, now seeing "later" issues? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/133057
>From e5f64e795f10a81a009b4562db0de692deb4c0f7 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:27:59 +0100
Subject: [PATCH 1/5] [Serialization] Hash inner template arguments
The code is applied from ODRHash::AddDecl with the reasoning given
in the comment, to reduce collisions. This was particularly visible
with STL types templated on std::pair where its template arguments
were not taken into account.
---
.../lib/Serialization/TemplateArgumentHasher.cpp | 15 +++
1 file changed, 15 insertions(+)
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp
b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index 3e8ffea78c2f1..d2aef73dec723 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -202,6 +202,21 @@ void TemplateArgumentHasher::AddDecl(const Decl *D) {
}
AddDeclarationName(ND->getDeclName());
+
+ // If this was a specialization we should take into account its template
+ // arguments. This helps to reduce collisions coming when visiting template
+ // specialization types (eg. when processing type template arguments).
+ ArrayRef Args;
+ if (auto *CTSD = dyn_cast(D))
+Args = CTSD->getTemplateArgs().asArray();
+ else if (auto *VTSD = dyn_cast(D))
+Args = VTSD->getTemplateArgs().asArray();
+ else if (auto *FD = dyn_cast(D))
+if (FD->getTemplateSpecializationArgs())
+ Args = FD->getTemplateSpecializationArgs()->asArray();
+
+ for (auto &TA : Args)
+AddTemplateArgument(TA);
}
void TemplateArgumentHasher::AddQualType(QualType T) {
>From e0dbf8e9c8ac320555181591ad4e30cb1ee5a926 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:31:23 +0100
Subject: [PATCH 2/5] [Serialization] Complete only needed partial
specializations
It is unclear (to me) why this needs to be done "for safety", but
this change significantly improves the effectiveness of lazy loading.
---
clang/lib/Serialization/ASTReader.cpp | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index e8a584a9b..f7c4e1846393b 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8089,14 +8089,8 @@ void ASTReader::CompleteRedeclChain(const Decl *D) {
}
}
- if (Template) {
-// For partitial specialization, load all the specializations for safety.
-if (isa(D))
- Template->loadLazySpecializationsImpl();
-else
- Template->loadLazySpecializationsImpl(Args);
- }
+ if (Template)
+Template->loadLazySpecializationsImpl(Args);
}
CXXCtorInitializer **
>From 9325b153a46e272f2627a341097ff9527a8837b4 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:42:20 +0100
Subject: [PATCH 3/5] [Serialization] Load only needed partial specializations
Similar as the last commit, it is unclear why we need to load all
specializations, including non-partial ones, when we have a TPL.
---
clang/lib/AST/DeclTemplate.cpp | 6 --
1 file changed, 6 deletions(-)
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 8a6fb3391c13f..6b918c2d75abb 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -368,12 +368,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl(
if (!ExternalSource)
return false;
- // If TPL is not null, it implies that we're loading specializations for
- // partial templates. We need to load all specializations in such cases.
- if (TPL)
-return
ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
- /*OnlyPartial=*/false);
-
return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
Args);
}
>From 3d95698ac96a9db6e99b02741d998fcce2a98735 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 25 Mar 2025 11:59:46 +0100
Subject: [PATCH 4/5] [Serialization] Remove bail-out logic in
TemplateArgumentHasher
While it is correct to assign a single fixed hash to all template
arguments, it can reduce the effectiveness of lazy loading and is
not actually needed: we are allowed to ignore parts that cannot be
handled because they will be analogously ignored by all hashings.
---
.../Serialization/TemplateArgumentHasher.cpp | 36 ++-
1 file changed, 3 insertions(+), 33 deletions(-)
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp
b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index d2aef73dec723..70f963b63509c 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -22,17 +22,6 @@ using namespace clang;
namespace {
class TemplateArgumentHasher {
- // If
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: Invested some time today and I think the fix is https://github.com/llvm/llvm-project/pull/154158. It works for the reproducer posted in https://github.com/llvm/llvm-project/pull/133057#issuecomment-2886816972, @ilya-biryukov or @emaxx-google if you have some time, would it be possible to test the two PRs together in your infrastructure? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > @hahnjo, if we have a working reproducer could we see if @zygoloid's comment > helps us: > > > If this issue is indeed specific to conversion function templates, perhaps > > a path forward would be to disable some part of the lazy loading for only > > those templates to unblock this patch while the reason for the problem is > > being investigated? > > That would mean that we exclude from lazy serialization the templated > conversion functions. I replied to this suggestion in https://github.com/llvm/llvm-project/pull/133057#issuecomment-2889787645. That still stands, I don't see that anything changed since May. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > I don't think we should merge a partial state of this PR for two reasons: 1. > The three patches alone don't actually bring much benefit. When I measured in > ROOT, we definitely needed the (currently) problematic one for lazy template > loading to become really effective. 2. As commented before, I suspect that > the patch only reveals a problem that is already there right now. We manage > to trigger it with that change, but I think it must be understood first > before changing the baseline. @hahnjo, if we have a working reproducer could we see if @zygoloid's comment helps us: > If this issue is indeed specific to conversion function templates, perhaps a > path forward would be to disable some part of the lazy loading for only those > templates to unblock this patch while the reason for the problem is being > investigated? That would mean that we exclude from lazy serialization the templated conversion functions. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: I don't think we should merge a partial state of this PR for two reasons: 1. The three patches alone don't actually bring much benefit. When I measured in ROOT, we definitely needed the (currently) problematic one for lazy template loading to become really effective. 2. As commented before, I suspect that the patch only reveals a problem that is already there right now. We manage to trigger it with that change, but I think it must be understood first before changing the baseline. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
https://github.com/hahnjo converted_to_draft https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
https://github.com/ChuanqiXu9 approved this pull request. BTW, I've landed the 3 patches (excluding the problematic reported by Google) internally and we've used it for months. It looks good. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > > I'm not so sure it's about conversion function templates in particular: if > > it's really commit > > [658d55b](https://github.com/llvm/llvm-project/commit/658d55ba1117a029f37f51a3072585a1fd9fc59f) > > causing the problem (@emaxx-google if you happen to have some time, it > > would be great to confirm reverting that change also avoids the issues in > > the original case), then there should only be a change for > > `ClassTemplatePartialSpecializationDecl` and > > `VarTemplatePartialSpecializationDecl`. > > I confirmed that reverting > [[658d55b](https://github.com/llvm/llvm-project/commit/658d55ba1117a029f37f51a3072585a1fd9fc59f)] > results in a successful compilation of the original google3 target. @hahnjo, can we move forward with at least the other changes? I'd like to converge with this soon since it has quite good benefits from the modules users perspective. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: BTW, I don't mind landing the rest 3 patches since they are "verified". https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > ### Performance measurements with LLVM > I tested these patches for building LLVM itself with modules > (`LLVM_ENABLE_MODULES=ON`). To work around #130795, I apply #131354 before > building Clang. In terms of overall performance for the entire build, I'm not > able to measure a difference in memory consumption because that is dominated > by the linker. The run time performance is very noisy, so it's hard to make > accurate statements but it looks unaffected as well. > > I did some measurements for individual files, chosen by searching for large > object files and excluding generated files. For each version, I first build > LLVM completely to populate the `module.cache` and then delete and rebuild > only one object file. Run time performance is not hugely affected, it seems > to get slightly faster with this PR. > > `Maximum resident set size (kbytes)` from `/usr/bin/time -v`: > > object file before* `main` this PR > `lib/Analysis/CMakeFiles/LLVMAnalysis.dir/ScalarEvolution.cpp.o` 543100 > 515184 445784 > `lib/Passes/CMakeFiles/LLVMPasses.dir/PassBuilder.cpp.o` 923036 884160 > 805960 > `lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o` > 639184 600076 522512 > `lib/Transforms/Vectorize/CMakeFiles/LLVMVectorize.dir/SLPVectorizer.cpp.o` > 876580 857404 776572 > before*: reverting > [fb2c9d9](https://github.com/llvm/llvm-project/commit/fb2c9d940ad87e6ae09e06c6915e0c925a4f87ec), > > [c5e4afe](https://github.com/llvm/llvm-project/commit/c5e4afe6733c58e24023ede04275bbed3bde8240), > > [30ea0f0](https://github.com/llvm/llvm-project/commit/30ea0f0ce476bf4c12684a9a514af2ca660bbe44), > > [20e9049](https://github.com/llvm/llvm-project/commit/20e904950967c125abc1e91f57e5a373987ff016) > on current `main` Out of curiosity, are you saying header modules are helpful to reduce the size of objects? This makes me surprise a little bit. I thought they are only helpful for parsing but not code generating. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: > I'm not so sure it's about conversion function templates in particular: if > it's really commit > [658d55b](https://github.com/llvm/llvm-project/commit/658d55ba1117a029f37f51a3072585a1fd9fc59f) > causing the problem (@emaxx-google if you happen to have some time, it would > be great to confirm reverting that change also avoids the issues in the > original case), then there should only be a change for > `ClassTemplatePartialSpecializationDecl` and > `VarTemplatePartialSpecializationDecl`. I confirmed that reverting [658d55b] results in a successful compilation of the original google3 target. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ilya-biryukov wrote: I can confirm that we routinely rely on the same header being part of multiple modules and results of that being combined in all kinds of ways imaginable. Moreover, the same header can be a modular header in some modules, textual in others and the resulting PCMs may also be combined together later (i.e. used as a direct or transitive dependencies of another compilation). Clang currently supports that. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: > Two observations: > > 1. There is a diamond module structure: Both `4BK.pcm` and `LUM.pcm` depend > on `WI9.pcm` and contain `2OT.h` with different module names (I removed > transitive empty includes). I'm not sure if this is valid... @emaxx-google is > it possible to share the modulemap for Abseil? We should check if all private > headers that are potentially used by multiple "public" modules are correctly > taken care of. Not sure if I get the question correctly, but `2OT.h` is a non-modular textual header. Yes, it happens to be #included from two places which end up compiled into different modules, but that should be OK?.. These modules don't originate from Abseil: `WI9.pcm` comes from STL; `4BK.pcm` and `LUM.pcm` are some google3 libraries. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: I'm not so sure it's about conversion function templates in particular: if it's really commit 658d55ba1117a029f37f51a3072585a1fd9fc59f causing the problem (@emaxx-google if you happen to have some time, it would be great to confirm reverting that change also avoids the issues in the original case), then there should only be a change for `ClassTemplatePartialSpecializationDecl` and `VarTemplatePartialSpecializationDecl`. In the reduced reproducer (https://github.com/llvm/llvm-project/pull/133057#issuecomment-2886816972), I only see a single partial specialization of `struct vy`. To me the connection to the conversion operator of `me` is very unclear, but I'm definitely not an expert in Clang's template instantiation machinery... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
zygoloid wrote: Conversion function templates might be a special case here because of how conversion function template specializations are found by deduction as part of class member name lookup. Maybe we're doing something different in that deduction in particular that means we're not properly lazily loading them? If this issue is indeed specific to conversion function templates, perhaps a path forward would be to disable some part of the lazy loading for only those templates to unblock this patch while the reason for the problem is being investigated? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote:
Thanks, the reproducer is indeed useful. The first "bad" commit is the second
`Complete only needed partial specializations`.
I was manually able to further reduce the example (throwing out empty files &
modules, inline through typedefs, etc)
Smaller reproducer
```
//--- 2OT.h
#include "LQ1.h"
namespace ciy {
namespace xqk {
template
class vum {
public:
using sc = std::C::wmd;
friend bool operator==(vum, vum);
};
template
class me {
public:
using vbh = vum;
using sc = std::C::vy::sc;
template
operator db() { return {}; }
};
} // namespace xqk
template
xqk::me uvo(std::C::wmd, vus);
} // namespace ciy
class ua {
std::C::wmd kij() {
ciy::uvo(kij(), '-');
return {};
}
};
//--- 9KF.h
#include "LQ1.h"
#include "2OT.h"
namespace {
void al(std::C::wmd lou) { std::C::jv yt = ciy::uvo(lou, '/'); }
} // namespace
//--- CMO.cppmap
module "hf" {
header "LQ1.h"
}
//--- E6H.cppmap
module "g" {
export *
header "2OT.h"
}
//--- HMT.cppmap
module "r" {
header "2OT.h"
}
//--- JOV.cppmap
module "q" {
header "9KF.h"
}
//--- LQ1.h
namespace std {
namespace C {
template
struct vy : zd {};
template
struct vy {
typedef ub jz;
};
struct wmd {};
template
void sk(uo k, zt gf) {
(void)(k != gf);
}
template
class fm {
public:
fm(uo);
};
template
bool operator==(kj, kju);
template
void afm(epn) {
using yp = vy;
if (__is_trivially_copyable(yp)) {
sk(fm(epn()), nullptr);
}
}
template
class jv {
public:
constexpr void gq();
ub *nef;
};
template
constexpr void jv::gq() {
afm(nef);
}
} // namespace C
} // namespace std
namespace ciy {
} // namespace ciy
//--- Makefile
.ALWAYS:
FZV.o: .ALWAYS WI9.pcm 4BK.pcm LUM.pcm 9VX.pcm
$(CLANG) -fmodule-name=p -Xclang=-fno-cxx-modules -fmodules
-fno-implicit-modules -fno-implicit-module-maps -Xclang=-fmodule-file=9VX.pcm
-fmodule-map-file=CMO.cppmap -std=gnu++20 -c XFD.cc -o FZV.o
WI9.pcm: .ALWAYS
$(CLANG) -fmodule-name=hf -fmodule-map-file=CMO.cppmap
-Xclang=-fno-cxx-modules -xc++ -Xclang=-emit-module -fmodules
-fno-implicit-modules -fno-implicit-module-maps -std=gnu++20 -c CMO.cppmap -o
WI9.pcm
4BK.pcm: .ALWAYS WI9.pcm
$(CLANG) -fmodule-name=g -fmodule-map-file=E6H.cppmap
-Xclang=-fno-cxx-modules -xc++ -Xclang=-emit-module -fmodules
-fno-implicit-modules -fno-implicit-module-maps -Xclang=-fmodule-file=WI9.pcm
-fmodule-map-file=CMO.cppmap -std=gnu++20 -c E6H.cppmap -o 4BK.pcm
LUM.pcm: .ALWAYS WI9.pcm
$(CLANG) -fmodule-name=r -fmodule-map-file=HMT.cppmap
-Xclang=-fno-cxx-modules -xc++ -Xclang=-emit-module -fmodules
-fno-implicit-modules -fno-implicit-module-maps -Xclang=-fmodule-file=WI9.pcm
-fmodule-map-file=CMO.cppmap -std=gnu++20 -c HMT.cppmap -o LUM.pcm
9VX.pcm: .ALWAYS WI9.pcm 4BK.pcm LUM.pcm
$(CLANG) -fmodule-name=q -fmodule-map-file=JOV.cppmap
-Xclang=-fno-cxx-modules -xc++ -Xclang=-emit-module -fmodules
-fno-implicit-modules -fno-implicit-module-maps -Xclang=-fmodule-file=LUM.pcm
-Xclang=-fmodule-file=4BK.pcm -fmodule-map-file=CMO.cppmap -std=gnu++20 -c
JOV.cppmap -o 9VX.pcm
.PHONY: clean
clean:
find . \( -name '*.pcm' -o -name '*.o' \) -delete
//--- XFD.cc
#include "LQ1.h"
#include "2OT.h"
class wiy {
public:
std::C::wmd eyb();
};
template
void i(wpa fg) {
std::C::jv zs;
zs = ciy::uvo(fg.eyb(), '\n');
}
namespace ciy {
namespace xqk {
struct sbv;
std::C::jv ns() {
std::C::jv ubs;
ubs.gq();
return ubs;
}
} // namespace xqk
} // namespace ciy
void s() {
wiy fg;
i(fg);
}
```
The failure with modules and this PR is:
```
XFD.cc:10:6: error: use of overloaded operator '=' is ambiguous (with operand
types 'std::C::jv' and 'xqk::me')
10 | zs = ciy::uvo(fg.eyb(), '\n');
| ~~ ^
XFD.cc:24:3: note: in instantiation of function template specialization
'i' requested here
24 | i(fg);
| ^
./LQ1.h:29:7: note: candidate function (the implicit copy assignment operator)
29 | class jv {
| ^
./LQ1.h:29:7: note: candidate function (the implicit move assignment operator)
```
Two observations:
1. There is a diamond module structure: Both `4BK.pcm` and `LUM.pcm` depend on
`WI9.pcm` and contain `2OT.h` with different module names (I removed transitive
empty includes). I'm not sure if this is valid... @emaxx-google is it possible
to share the modulemap for Abseil? We should check if all private headers that
are potentially used by multiple "public" modules are correctly taken care of.
2. The problem is the conversion operator `xqk::me` that is declared as
`template`. I *think* the problem is that an object can be implicitly converted
into multiple temporaries that can either be copy- or move-constructed.
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: Hello, here's the new reproducer (which compiles "before" and also in non-module mode, but not "after"): https://pastebin.com/zawQv7Q6 . Hopefully this time it's more useful than the previous attempts. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > @emaxx-google, @ilya-biryukov, do you think we can move forward even though > don't have a reproducer yet? It feels we are stuck on that one case on a > downstream client -- worst case maybe the devs can work it around by changing > source code for that one case? Yes, I'm in favor of this. I feel it, more or less, doesn't make sense to wait for a downstream without a reproducer without a DDL ... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
@@ -367,12 +367,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl(
if (!ExternalSource)
return false;
- // If TPL is not null, it implies that we're loading specializations for
- // partial templates. We need to load all specializations in such cases.
- if (TPL)
hahnjo wrote:
I tried to reproduce the problem, but it's not enough information about the
setup yet. First, https://github.com/abseil/abseil-cpp/ doesn't have a
`modulemap`, so I created one myself:
```
module absl {
module string {
module string_view {
header "install/include/absl/strings/string_view.h"
export *
}
module str_split {
header "install/include/absl/strings/str_split.h"
export *
}
}
}
```
Where do you get this from in your case and how do you compile the library +
user code? The small snippet I tried was:
```c++
#include
#include
void test() {
std::vector views;
std::vector& x = views;
x = absl::StrSplit("abc", 'y');
}
```
> Unfortunately the error in Google3 doesn't go away after reverting
> https://github.com/llvm/llvm-project/commit/48f50c7385d79da320cdf52d3aabc83cc403ff9c.
Ok, was worth a shot. Is it possible to figure out which of the four commits
causes the problems? Then at least we could get some information what is
causing it.
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: An update: The miminization of the reproducer is sadly still ongoing. A longer version of this TLDR: --- As the reduction was making slow progress, I've tried the proposal from @vgvassilev to add the "successfully compiles in the non-modular mode" criterion to the interestingness test. It did let the reduction finish in just a couple of days, but the result turned out to suffer from the same problems as before: lots of broken code and commands, scattered through the files not "reachable" in non-modular builds (for headers only mentioned in cppmaps but not included directly or transitively). Hence I returned back to the idea with minimizing without `-fallow-pcm-with-compiler-errors`. I improved a few reduction heuristics, but also the progress stalled a few times due to various issues (disk space, OOMs and compiler hangs, bugs in the new heuristics). It looks like it's a very tough input for the C-Reduce-like minimization, but unfortunately it's the only viable approach we have at the moment. I'll provide an update here once I have something to share (likely in a few days). Thanks for the patience! https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
@@ -367,12 +367,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl( if (!ExternalSource) return false; - // If TPL is not null, it implies that we're loading specializations for - // partial templates. We need to load all specializations in such cases. - if (TPL) emaxx-google wrote: Unfortunately the error in Google3 doesn't go away after reverting https://github.com/llvm/llvm-project/commit/48f50c7385d79da320cdf52d3aabc83cc403ff9c. The failures do happen on overloaded template functions. Actually we can disclose some of the information about the failing line - where the STL and Abseil are involved: ```c++ std::vector& x = some_vector_of_views; x = absl::StrSplit(another_vector_of_views, 'y'); // <--- triggers "use of overloaded operator '=' is ambiguous" ``` Hopefully it's helpful, although this snippet looks very simple, and the root cause probably sits elsewhere. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: LGTM. Let's land this in individual commits after the large test in google finish. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
@@ -21,17 +21,6 @@ using namespace clang;
namespace {
class TemplateArgumentHasher {
- // If we bail out during the process of calculating hash values for
- // template arguments for any reason. We're allowed to do it since
- // TemplateArgumentHasher are only required to give the same hash value
- // for the same template arguments, but not required to give different
- // hash value for different template arguments.
- //
- // So in the worst case, it is still a valid implementation to give all
- // inputs the same BailedOutValue as output.
- bool BailedOut = false;
- static constexpr unsigned BailedOutValue = 0x12345678;
ChuanqiXu9 wrote:
Not yet. We can split this in two commits, then if anything bad happens, we can
revert precisely.
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
@@ -367,12 +367,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl( if (!ExternalSource) return false; - // If TPL is not null, it implies that we're loading specializations for - // partial templates. We need to load all specializations in such cases. - if (TPL) hahnjo wrote: Actually, looking into df061c3e2b97974f9e2d72a023eb1c5b987010bc this may be something which is not properly handled for lazy template loading. @emaxx-google can you check if the failures you are seeing involves constrained partial specializations or function overloads? If so, can you mayble locally revert 48f50c7385d79da320cdf52d3aabc83cc403ff9c `[Serialization] Load only needed partial specializations`? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
@@ -21,17 +21,6 @@ using namespace clang;
namespace {
class TemplateArgumentHasher {
- // If we bail out during the process of calculating hash values for
- // template arguments for any reason. We're allowed to do it since
- // TemplateArgumentHasher are only required to give the same hash value
- // for the same template arguments, but not required to give different
- // hash value for different template arguments.
- //
- // So in the worst case, it is still a valid implementation to give all
- // inputs the same BailedOutValue as output.
- bool BailedOut = false;
- static constexpr unsigned BailedOutValue = 0x12345678;
hahnjo wrote:
Sure, the hasher must skip all inputs that could differ in their literal
spelling, but I don't see why we have to hash them to a single global value. If
two semantically same Decls / Types are not literally the same, we skip the
input for both of them and the output hash will still be the same.
Do you have a concrete example in mind that you think should be tested?
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
@@ -367,12 +367,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl( if (!ExternalSource) return false; - // If TPL is not null, it implies that we're loading specializations for - // partial templates. We need to load all specializations in such cases. - if (TPL) hahnjo wrote: Yes, we could remove it from `loadLazySpecializationsImpl`, but we need it in `Profile` since df061c3e2b97974f9e2d72a023eb1c5b987010bc. This complicates the removal considerably, which IMHO makes the code more complicated: https://github.com/hahnjo/llvm-project/commit/bb4788afc66690975e67c7cf1461033e79feb13f https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: Understood. JFYI I've been running another minimization task, this time without the `-fallow-pcm-with-compiler-errors` parameter, so that all modules compile correctly and the result is easier to comprehend, but it obviously takes a lot longer to reduce - so far it's still at 100 MB after running for 3 days (the start size was 0.5 GB). We do have tools to filter out unrelated files, still the initial input size is that big. But we continue looking into various aspects of the minimization performance. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > When I have time, I will need to see that I turn the reduced example into > actual valid code to understand what is going on... I spent some time turning it into valid code, but then noticed that the `error: use of overloaded operator '=' is ambiguous` is gone. Going back, the piece that triggers the error is the token `bns` in `TR7.cc`, which leads to `error: unknown type name 'bns'` and then messes up parsing of `namespace sr` in `UL8.h`. I believe the reproducer is not useful in the current form, you will have to instruct the reduce process to keep somewhat valid code... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > > > Here's a new reproducer, this time verifying that a semi-fresh > > > tip-of-the-tree doesn't trigger the same error: > > > https://pastebin.com/7tYfjazz. Apologies for the delay. > > > > > > Thanks. I gave it a try, but I don't see any `use of overloaded operator > > '=' is ambiguous` error. In fact, there isn't a `operator =` in the sources > > at all. What am I missing? 🤔 > > I didn't investigate the error - but could it come from autogenerated > operators? > > > Creduce-like tools are very bad at finding reject valid codes. > > @emaxx-google, can you paste your interestingness test? > > Here's the (slightly smaller) input: https://pastebin.com/dBy1T9fc . The > simplified script: https://pastebin.com/udVTaPYV . Its output: > https://pastebin.com/Mcq0Y0rf . A reduce strategy that often worked for me is to have the broken compiler in modules mode but the working compiler in non-modules mode. If you prepend a tool such as https://github.com/Teemperor/hippie you can get all read by the compiler files in a readable area. Then you can use (-isysroot or the same tool to feed them back to the compiler). In this case you will have a "flat" TU which can run in both modules and non-modules mode. I hope that helps with that case, too... https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote:
> The simplified script: https://pastebin.com/udVTaPYV
Aaaah, the important line is `EXTRA_CFLAGS='-Xclang
-fallow-pcm-with-compiler-errors -ferror-limit=0'`. With that I indeed get the
following diff between logs from `main` and a rebased version of this branch:
```diff
--- main.log2025-04-11 14:31:26.655249634 +0200
+++ clang-modules-rebased.log 2025-04-11 14:30:12.184305904 +0200
@@ -417,12 +417,15 @@
TR7.cc:4:1: error: a type specifier is required for all declarations
4 | std (
| ^
-TR7.cc:8:1: error: 'operator type-parameter-0-0' is a private member of
'sr::g'
+TR7.cc:7:3: error: use of overloaded operator '=' is ambiguous (with operand
types 'uz' and 'g')
+7 | q =
+ | ~ ^
8 | dp(h, '\n')
- | ^~~
-./UL8.h:6:1: note: implicitly declared private here
-6 | operator pzx(;
- | ^
+ | ~~~
+L3Z:31:7: note: candidate function (the implicit copy assignment operator)
+ 31 | class uz {typede x t;
+ | ^
+L3Z:31:7: note: candidate function (the implicit move assignment operator)
TR7.cc:8:12: error: expected ';' after expression
8 | dp(h, '\n')
|^
```
When I have time, I will need to see that I turn the reduced example into
actual valid code to understand what is going on...
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: > > Here's a new reproducer, this time verifying that a semi-fresh > > tip-of-the-tree doesn't trigger the same error: > > https://pastebin.com/7tYfjazz. Apologies for the delay. > > Thanks. I gave it a try, but I don't see any `use of overloaded operator '=' > is ambiguous` error. In fact, there isn't a `operator =` in the sources at > all. What am I missing? 🤔 I didn't investigate the error - but could it come from autogenerated operators? > Creduce-like tools are very bad at finding reject valid codes. @emaxx-google, > can you paste your interestingness test? Here's the (slightly smaller) input: https://pastebin.com/dBy1T9fc . The simplified script: https://pastebin.com/udVTaPYV . Its output: https://pastebin.com/Mcq0Y0rf . https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > > Here's a new reproducer, this time verifying that a semi-fresh > > tip-of-the-tree doesn't trigger the same error: > > https://pastebin.com/7tYfjazz. Apologies for the delay. > > Thanks. I gave it a try, but I don't see any `use of overloaded operator '=' > is ambiguous` error. In fact, there isn't a `operator =` in the sources at > all. What am I missing? 🤔 Creduce-like tools are very bad at finding reject valid codes. @emaxx-google, can you paste your interestingness test? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: > Here's a new reproducer, this time verifying that a semi-fresh > tip-of-the-tree doesn't trigger the same error: > https://pastebin.com/7tYfjazz. Apologies for the delay. Thanks. I gave it a try, but I don't see any `use of overloaded operator '=' is ambiguous` error. In fact, there isn't a `operator =` in the sources at all. What am I missing? :thinking: https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: Here's a new reproducer, this time verifying that a semi-fresh tip-of-the-tree doesn't trigger the same error: https://pastebin.com/7tYfjazz. Apologies for the delay. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote: Hi @emaxx-google, any updates on the second minimization run? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
emaxx-google wrote: > I had a closer look, but I get plenty of compile errors already on `main` - > including > > ``` > ./head15.h:20:7: error: use of overloaded operator '=' is ambiguous (with > operand types 'std::vector' and > 'strings_internal::Splitter strings_internal::SelectDelimiter::type, AllowEmpty, std::string>' (aka > 'Splitter')) > ``` > > I haven't even applied the change in this PR - what am I missing? You're right - before sharing the reproducer I cross-checked that the error isn't produced by a slightly older version of Clang (a few weeks old), but I didn't check the most recent version. Apparently something changed in the last few weeks that affects this error regardless of your change. OK, I'm re-running the minimization now, this time with the interestingness test being "the given error is produced by compiler1 and not produced by compiler2=dd3addf954ac7e704fccc7d011217ba10461c883". https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ilya-biryukov wrote: The small-scale benchmarks we had show 10% improvement in CPU and 23% improvement in memory usage for some compilations! We did hit one compiler error that does not reproduce without modules, however: ```error: use of overloaded operator '=' is ambiguous``` We're in the process of getting a small reproducer (please bear with us, it takes some time) that we can share. @emaxx-google is working on it. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > The small-scale benchmarks we had show 10% improvement in CPU and 23% > improvement in memory usage for some compilations! That's very good news. I think we can further reduce these times. IIRC, we still deserialize declarations that we do not need. One of the places to look is the logic that kicks in when at module loading time: https://github.com/llvm/llvm-project/blob/772173f54868eef6e1a4d40ab93b0ee6c04b1aca/clang/lib/Serialization/ASTReader.cpp#L3426 > > We did hit one compiler error that does not reproduce without modules, > however: `error: use of overloaded operator '=' is ambiguous` Ouch. If that's the only issue on your infrastructure that's probably not so bad. > > We're in the process of getting a small reproducer (please bear with us, it > takes some time) that we can share. @emaxx-google is working on it. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > > > While I may not able to look into them in detail recently, it may be > > > helpful to split this into seperate patches to review and to land. > > > > > > I initially considered this, but @vgvassilev said in > > [root-project/root#17722 > > (comment)](https://github.com/root-project/root/pull/17722#issuecomment-2706555950) > > he prefers a single PR, also for external testing. > > Maybe you can test it with this and land it with different patches. So that > we can revert one of them if either of them are problematic but other parts > are fine. This is a relatively small patch focused on reducing the round trips to modules deserialization. I see this as an atomic change that if it goes in partially would defeat its purpose. What's the goal of a partial optimization? https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > > > > > While I may not able to look into them in detail recently, it may > > > > > > be helpful to split this into seperate patches to review and to > > > > > > land. > > > > > > > > > > > > > > > I initially considered this, but @vgvassilev said in > > > > > [root-project/root#17722 > > > > > (comment)](https://github.com/root-project/root/pull/17722#issuecomment-2706555950) > > > > > he prefers a single PR, also for external testing. > > > > > > > > > > > > Maybe you can test it with this and land it with different patches. So > > > > that we can revert one of them if either of them are problematic but > > > > other parts are fine. > > > > > > > > > This is a relatively small patch focused on reducing the round trips to > > > modules deserialization. I see this as an atomic change that if it goes > > > in partially would defeat its purpose. What's the goal of a partial > > > optimization? > > > > > > I think partial optimizations are optimization too. If these codes are not > > dependent on each other, it should be better to split them. > > Given the scale of the patch, it may not be serious problem actually. I > > still think it is better to land them separately, but if you want to save > > some typings. I don't feel too bad. > > Honestly I am more concerned about the tests that @ilya-biryukov is running. > As long as they are happy I do not particularly care about commit style. > Although it'd be weird to land 40 line patch in many commits :) I don't feel odd. I remember it is (or was) LLVM's policy that smaller patches are preferred : ) https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
vgvassilev wrote: > > > > > While I may not able to look into them in detail recently, it may be > > > > > helpful to split this into seperate patches to review and to land. > > > > > > > > > > > > I initially considered this, but @vgvassilev said in > > > > [root-project/root#17722 > > > > (comment)](https://github.com/root-project/root/pull/17722#issuecomment-2706555950) > > > > he prefers a single PR, also for external testing. > > > > > > > > > Maybe you can test it with this and land it with different patches. So > > > that we can revert one of them if either of them are problematic but > > > other parts are fine. > > > > > > This is a relatively small patch focused on reducing the round trips to > > modules deserialization. I see this as an atomic change that if it goes in > > partially would defeat its purpose. What's the goal of a partial > > optimization? > > I think partial optimizations are optimization too. If these codes are not > dependent on each other, it should be better to split them. > > Given the scale of the patch, it may not be serious problem actually. I still > think it is better to land them separately, but if you want to save some > typings. I don't feel too bad. Honestly I am more concerned about the tests that @ilya-biryukov is running. As long as they are happy I do not particularly care about commit style. Although it'd be weird to land 40 line patch in many commits :) https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > > > While I may not able to look into them in detail recently, it may be > > > > helpful to split this into seperate patches to review and to land. > > > > > > > > > I initially considered this, but @vgvassilev said in > > > [root-project/root#17722 > > > (comment)](https://github.com/root-project/root/pull/17722#issuecomment-2706555950) > > > he prefers a single PR, also for external testing. > > > > > > Maybe you can test it with this and land it with different patches. So that > > we can revert one of them if either of them are problematic but other parts > > are fine. > > This is a relatively small patch focused on reducing the round trips to > modules deserialization. I see this as an atomic change that if it goes in > partially would defeat its purpose. What's the goal of a partial optimization? I think partial optimizations are optimization too. If these codes are not dependent on each other, it should be better to split them. Given the scale of the patch, it may not be serious problem actually. I still think it is better to land them separately, but if you want to save some typings. I don't feel too bad. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote:
> > Maybe you can test it with this and land it with different patches. So that
> > we can revert one of them if either of them are problematic but other parts
> > are fine.
>
> I'm ok with pushing the commits one-by-one after the PR is reviewed, just let
> me know.
>
> > > Complete only needed partial specializations: It is unclear (to me) why
> > > this needs to be done "for safety", but this change significantly
> > > improves the effectiveness of lazy loading.
> >
> >
> > This comes from the logic: if we have a partial template specialization
> > `A` and we need a full specialization for `A > double>`, we hope the partial specialization to be loaded
>
> Sure, but in my understanding, that's not needed on the `ASTReader` side but
> is taken care of by `Sema` (?). For the following example:
>
> ```c++
> //--- partial.cppm
> export module partial;
>
> export template
> struct Partial {
> static constexpr int Value() { return 0; }
> };
>
> export template
> struct Partial {
> static constexpr int Value() { return 1; }
> };
>
> //--- partial.cpp
> import partial;
>
> static_assert(Partial::Value() == 1);
> ```
>
> (I assume that's what you have in mind?) I see two calls to
> `ASTReader::CompleteRedeclChain` (with this PR applied): The first asks for
> the full instantiation `Partial` and regardless of what
> we load, the answer to the query is that it's not defined yet. The second
> asks for the partial specialization `Partial` and then
> instantiation proceeds to do the right thing.
If it works, I feel good with it.
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
hahnjo wrote:
> Maybe you can test it with this and land it with different patches. So that
> we can revert one of them if either of them are problematic but other parts
> are fine.
I'm ok with pushing the commits one-by-one after the PR is reviewed, just let
me know.
> > Complete only needed partial specializations: It is unclear (to me) why
> > this needs to be done "for safety", but this change significantly improves
> > the effectiveness of lazy loading.
>
> This comes from the logic: if we have a partial template specialization
> `A` and we need a full specialization for `A double>`, we hope the partial specialization to be loaded
Sure, but in my understanding, that's not needed on the `ASTReader` side but is
taken care of by `Sema` (?). For the following example:
```c++
//--- partial.cppm
export module partial;
export template
struct Partial {
static constexpr int Value() { return 0; }
};
export template
struct Partial {
static constexpr int Value() { return 1; }
};
//--- partial.cpp
import partial;
static_assert(Partial::Value() == 1);
```
(I assume that's what you have in mind?) I see two calls to
`ASTReader::CompleteRedeclChain` (with this PR applied): The first asks for the
full instantiation `Partial` and regardless of what we
load, the answer to the query is that it's not defined yet. The second asks for
the partial specialization `Partial` and then instantiation proceeds
to do the right thing.
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > Complete only needed partial specializations: It is unclear (to me) why this > needs to be done "for safety", but this change significantly improves the > effectiveness of lazy loading. This comes from the logic: if we have a partial template specialization `A` and we need a full specialization for `A`, we hope the partial specialization to be loaded https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Serialization] Fix lazy template loading (PR #133057)
ChuanqiXu9 wrote: > > While I may not able to look into them in detail recently, it may be > > helpful to split this into seperate patches to review and to land. > > I initially considered this, but @vgvassilev said in [root-project/root#17722 > (comment)](https://github.com/root-project/root/pull/17722#issuecomment-2706555950) > he prefers a single PR, also for external testing. Maybe you can test it with this and land it with different patches. So that we can revert one of them if either of them are problematic but other parts are fine. https://github.com/llvm/llvm-project/pull/133057 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
