ChuanqiXu9 wrote:
Given we're pursuing https://github.com/llvm/llvm-project/pull/83237 series.
I'll close this one.
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://github.com/ChuanqiXu9 closed
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/iains commented:
I am happy to defer to @vgvassilev et al. on this one.
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ChuanqiXu9 wrote:
> That'd mean that we "just" need to replace LazySpecializationInfo
> *LazySpecializations = nullptr; with the on-disk hash table approach. That
> would probably require centralizing that logic somewhere in the ASTReader
> (the way this PR does) but with minimal changes wrt
ChuanqiXu9 wrote:
Oh, I didn't notice you've removed D153003 already. But the branch name looks
not good. So I've created a pr in
https://github.com/llvm/llvm-project/pull/83108
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing
ChuanqiXu9 wrote:
> > > > Let's zoom out a little. The approach in D41416 shows that it is
> > > > feasible to store _a_ hash of the template arguments to delay eager
> > > > deserializations. The ODR hash approach is a second order problem
> > > > because we can swap it with something better
vgvassilev wrote:
> > > Let's zoom out a little. The approach in D41416 shows that it is feasible
> > > to store _a_ hash of the template arguments to delay eager
> > > deserializations. The ODR hash approach is a second order problem because
> > > we can swap it with something better once we
vgvassilev wrote:
> Sorry for losing track of the discussion here. What is the current status
> here? Should we run another round of testing?
>
> Also, I see proposals to land the new behaviour under a flag and have it off
> by default. If that does not add a lot of complexity, that would
ilya-biryukov wrote:
Sorry for losing track of the discussion here. What is the current status here?
Should we run another round of testing?
Also, I see proposals to land the new behaviour under a flag and have it off by
default.
If that does not add a lot of complexity, that would definitely
vgvassilev wrote:
> > Let's zoom out a little. The approach in D41416 shows that it is feasible
> > to store _a_ hash of the template arguments to delay eager
> > deserializations. The ODR hash approach is a second order problem because
> > we can swap it with something better once we need
ChuanqiXu9 wrote:
> Let's zoom out a little. The approach in D41416 shows that it is feasible to
> store _a_ hash of the template arguments to delay eager deserializations. The
> ODR hash approach is a second order problem because we can swap it with
> something better once we need to. In
vgvassilev wrote:
Let's zoom out a little. The approach in D41416 shows that it is feasible to
store *a* hash of the template arguments to delay eager deserializations. The
ODR hash approach is a second order problem because we can swap it with
something better once we need to. In order to
ChuanqiXu9 wrote:
> > But my point is that we can't land that if we don't understand what's going
> > wrong without that patch.
>
> We understand that very well and it's described in
> https://reviews.llvm.org/D153003 as well as the surrounding discussions:
> because of the way that
hahnjo wrote:
> But my point is that we can't land that if we don't understand what's going
> wrong without that patch.
We understand that very well and it's described in
https://reviews.llvm.org/D153003 as well as the surrounding discussions:
because of the way that `ODRHash` works,
ChuanqiXu9 wrote:
> > > > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy
> > > > > > > > > > template specialization deserialization mechanism
> > > > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > > >
> > > > > > > > >
>
vgvassilev wrote:
> > > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > > > > specialization deserialization mechanism
> > > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > >
> > > > > > > >
> > > > > >
ChuanqiXu9 wrote:
> > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > > > specialization deserialization mechanism
> > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > >
> > > > > > >
> > > > > > > From
>
vgvassilev wrote:
> > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > > specialization deserialization mechanism
> > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > >
> > > > > >
> > > > > > From
> > > > > >
ChuanqiXu9 wrote:
> > > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > > > > specialization deserialization mechanism
> > > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > >
> > > > > > > >
> > > > > >
vgvassilev wrote:
> > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > > > specialization deserialization mechanism
> > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > >
> > > > > > >
> > > > > > > From
>
ChuanqiXu9 wrote:
> > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > > specialization deserialization mechanism
> > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > >
> > > > > >
> > > > > > From
> > > > > >
vgvassilev wrote:
> > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > > specialization deserialization mechanism
> > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > >
> > > > >
> > > > > From
> > > > >
ChuanqiXu9 wrote:
> > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > > specialization deserialization mechanism
> > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > >
> > > >
> > > > From
> > > >
vgvassilev wrote:
> > > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > > specialization deserialization mechanism
> > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > >
> > >
> > > From
> > >
ChuanqiXu9 wrote:
> > > [do not merge] [runtime-cxxmodules] Rework our lazy template
> > > specialization deserialization mechanism
> > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> >
> >
> > From
> >
vgvassilev wrote:
> > [do not merge] [runtime-cxxmodules] Rework our lazy template specialization
> > deserialization mechanism
> > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
>
> From
>
ChuanqiXu9 wrote:
> [do not merge] [runtime-cxxmodules] Rework our lazy template specialization
> deserialization mechanism root-project/root#14495
>From https://github.com/root-project/root/pull/14495, I see there is new reply
>saying the testing is actually fine. Do you think we still need
ChuanqiXu9 wrote:
> This patch does too many things for me to be able to review it. This patch
> fails on our infrastructure.
>
> I'd propose to simplify it to basically D41416 + the on-disk hash table. We
> should read all of the entries upon module loading to simplify the logic in
>
@@ -603,21 +606,30 @@ class ASTReader
llvm::DenseMap Lookups;
+ /// Map from decls to specialized decls.
+ llvm::DenseMap
+ SpecializationsLookups;
vgvassilev wrote:
We should probably have a mapping between a template argument hash -> vector of
https://github.com/vgvassilev commented:
This patch does too many things for me to be able to review it. This patch
fails on our infrastructure.
I'd propose to simplify it to basically D41416 + the on-disk hash table. We
should read all of the entries upon module loading to simplify the logic
https://github.com/vgvassilev edited
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ChuanqiXu9 wrote:
I applied the change to https://github.com/ChuanqiXu9/root/tree/LoadSpecLazily
And the testing result looks like, hmm, unstable, there are 326 tests failed
out of 2174 and in the LoadSpecLazily version, there are 328 tests out of 2174.
But some tests passed in LoadSpecLazily
ChuanqiXu9 wrote:
> > > > > As far as I can tell from [#76774
> > > > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> > > > > above, the last push only changed the default value of
> > > > > `LoadExternalSpecializationsLazily`. In that case, my test
vgvassilev wrote:
> > > > As far as I can tell from [#76774
> > > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> > > > above, the last push only changed the default value of
> > > > `LoadExternalSpecializationsLazily`. In that case, my test results
ChuanqiXu9 wrote:
> > > As far as I can tell from [#76774
> > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> > > above, the last push only changed the default value of
> > > `LoadExternalSpecializationsLazily`. In that case, my test results from
> > >
vgvassilev wrote:
> > As far as I can tell from [#76774
> > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> > above, the last push only changed the default value of
> > `LoadExternalSpecializationsLazily`. In that case, my test results from
> > [#76774
ChuanqiXu9 wrote:
> As far as I can tell from [#76774
> (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> above, the last push only changed the default value of
> `LoadExternalSpecializationsLazily`. In that case, my test results from
> [#76774
>
vgvassilev wrote:
Ok, so it sounds it fails earlier than I expected. We might be missing
something in the implementation of the on-disk hashtable. Have you had a chance
to run valgrind?
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits
hahnjo wrote:
As far as I can tell from
https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330 above,
the last push only changed the default value of
`LoadExternalSpecializationsLazily`. In that case, my test results from
vgvassilev wrote:
> > The newest version of this patch still doesn't work correctly. Switching
> > the new `LoadExternalSpecializationsLazily` to disabled by default (somehow
> > the argument didn't work on its own) instead crashes, most of the cases
> > involving `MultiOnDiskHashTable`. I
ChuanqiXu9 wrote:
> The newest version of this patch still doesn't work correctly. Switching the
> new `LoadExternalSpecializationsLazily` to disabled by default (somehow the
> argument didn't work on its own) instead crashes, most of the cases involving
> `MultiOnDiskHashTable`. I suspect
vgvassilev wrote:
How about taking a step at a time with this patch. Perhaps we should introduce
the on-disk hash table infrastructure and always deserialize everything. Then
we can verify that part works on our build infrastructure and then move on with
the deferring the template loading. I
hahnjo wrote:
The newest version of this patch still doesn't work correctly. Switching the
new `LoadExternalSpecializationsLazily` to disabled by default (somehow the
argument didn't work on its own) instead crashes, most of the cases involving
`MultiOnDiskHashTable`. I suspect some kind of
https://github.com/ChuanqiXu9 updated
https://github.com/llvm/llvm-project/pull/76774
>From 26bd7dc5139811b2b0d8d8642a67b67340eeb1d5 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/4] Load Specializations Lazily
---
ChuanqiXu9 wrote:
In the newest push, I changed 2 things:
- In `ASTReader::LoadExternalSpecializations`, I am moving `Deserializing
LookupResults(this);` before calculating the ODR hash for template arguments.
Since I didn't realize that the ODRHash may trigger deserializing before.
- I
https://github.com/ChuanqiXu9 updated
https://github.com/llvm/llvm-project/pull/76774
>From 26bd7dc5139811b2b0d8d8642a67b67340eeb1d5 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/4] Load Specializations Lazily
---
hahnjo wrote:
> Would you like to give an reproducer so that I can debug?
Not easily at the moment, unless you want to go through building the entirety
of ROOT :-/
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
ChuanqiXu9 wrote:
> > > > I tried applying this patch to ROOT/Cling and it fails to build because
> > > > something is of the opinion that `std::is_integral::value` is not
> > > > true. I don't have time right now to investigate further, but since
> > > > this is the only change I did it is
hahnjo wrote:
> > > I tried applying this patch to ROOT/Cling and it fails to build because
> > > something is of the opinion that `std::is_integral::value` is not
> > > true. I don't have time right now to investigate further, but since this
> > > is the only change I did it is highly likely
ChuanqiXu9 wrote:
> > I tried applying this patch to ROOT/Cling and it fails to build because
> > something is of the opinion that `std::is_integral::value` is not
> > true. I don't have time right now to investigate further, but since this is
> > the only change I did it is highly likely
@@ -3924,6 +3925,154 @@ class ASTDeclContextNameLookupTrait {
} // namespace
+namespace {
+class SpecializationsLookupTrait {
+ ASTWriter
+ llvm::SmallVector DeclIDs;
+
+public:
+ using key_type = unsigned;
+ using key_type_ref = key_type;
+
+ /// A start and end index
@@ -3924,6 +3925,154 @@ class ASTDeclContextNameLookupTrait {
} // namespace
+namespace {
+class SpecializationsLookupTrait {
+ ASTWriter
+ llvm::SmallVector DeclIDs;
+
+public:
+ using key_type = unsigned;
+ using key_type_ref = key_type;
+
+ /// A start and end index
vgvassilev wrote:
> I tried applying this patch to ROOT/Cling and it fails to build because
> something is of the opinion that `std::is_integral::value` is not true.
> I don't have time right now to investigate further, but since this is the
> only change I did it is highly likely that it's
hahnjo wrote:
I tried applying this patch to ROOT/Cling and it fails to build because
something is of the opinion that `std::is_integral::value` is not true. I
don't have time right now to investigate further, but since this is the only
change I did it is highly likely that it's caused by
ChuanqiXu9 wrote:
> We saw some failures internally with this patch:
>
> * `"'absl::AnyInvocable' has different definitions in different
> modules;".`
> Probably related to the template arguments stability you mention.
> * `"No type named 'MemberType' in 'some_object::Traits'"`.
> I
ilya-biryukov wrote:
We saw some failures internally with this patch:
- ` "'absl::AnyInvocable' has different definitions in different
modules;". `
Probably related to the template arguments stability you mention.
- `"No type named 'MemberType' in 'some_object::Traits'"`.
I suspect that's
vgvassilev wrote:
> @dwblaikie do you have any memory of what caused regressions on the last try
> in the first place? Do we happen to do more work for compiles with many files
> because we accidentally push some computations further down the dependency
> tree and have to duplicate it instead
ChuanqiXu9 wrote:
> > @ilya-biryukov any chance you/your folks could test this change for
> > performance implications in google? It's especially helpful to CERN, but
> > the last iteration of this direction had some regressions that stalled out
> > progress on that version a few years ago,
ilya-biryukov wrote:
> @ilya-biryukov any chance you/your folks could test this change for
> performance implications in google? It's especially helpful to CERN, but the
> last iteration of this direction had some regressions that stalled out
> progress on that version a few years ago, so
https://github.com/ChuanqiXu9 updated
https://github.com/llvm/llvm-project/pull/76774
>From 50fd47f2bfda527807f8cc5e46425050246868aa Mon Sep 17 00:00:00 2001
From: Chuanqi Xu
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/3] Load Specializations Lazily
---
ChuanqiXu9 wrote:
Update:
Previously we will always try to load the specializations with the
corresponding arguments before finding the specializations. This
requires to hash the template arguments.
This patch tries to improve this by trying to load the specializations
only if
https://github.com/ChuanqiXu9 updated
https://github.com/llvm/llvm-project/pull/76774
>From 50fd47f2bfda527807f8cc5e46425050246868aa Mon Sep 17 00:00:00 2001
From: Chuanqi Xu
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/3] Load Specializations Lazily
---
ChuanqiXu9 wrote:
> @ChuanqiXu9, this PR does not seem to compile. Can you make the second commit
> work before I start testing?
Oh, sorry. It should work now!
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
https://github.com/ChuanqiXu9 updated
https://github.com/llvm/llvm-project/pull/76774
>From 50fd47f2bfda527807f8cc5e46425050246868aa Mon Sep 17 00:00:00 2001
From: Chuanqi Xu
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/2] Load Specializations Lazily
---
vgvassilev wrote:
@ChuanqiXu9, this PR does not seem to compile. Can you make the second commit
work before I start testing?
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ChuanqiXu9 wrote:
> Have you run that patch on bigger workflows? Do we have some performance
> numbers to compare?
I've tested it functionality in our largest workload about modules. It runs
well. But our uses of modules don't have a lot of complexities while it has a
large scale. For
@@ -100,6 +100,11 @@ ExternalASTSource::FindExternalVisibleDeclsByName(const
DeclContext *DC,
return false;
}
+void ExternalASTSource::LoadExternalSpecializations(
+const Decl *D, ArrayRef TemplateArgs) {
+ return;
ChuanqiXu9 wrote:
Will do in the
dwblaikie wrote:
@ilya-biryukov any chance you/your folks could test this change for performance
implications in google? It's especially helpful to CERN, but the last iteration
of this direction had some regressions that stalled out progress on that
version a few years ago, so it'd be good to
https://github.com/vgvassilev edited
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/vgvassilev commented:
Overall this looks quite promising to me. Have you run that patch on bigger
workflows? Do we have some performance numbers to compare?
I will run some tests on our infrastructure and report back.
https://github.com/llvm/llvm-project/pull/76774
@@ -100,6 +100,11 @@ ExternalASTSource::FindExternalVisibleDeclsByName(const
DeclContext *DC,
return false;
}
+void ExternalASTSource::LoadExternalSpecializations(
+const Decl *D, ArrayRef TemplateArgs) {
+ return;
vgvassilev wrote:
```suggestion
```
https://github.com/ChuanqiXu9 edited
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/ChuanqiXu9 edited
https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
73 matches
Mail list logo
