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 unbloc
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.
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 commen
https://github.com/hahnjo converted_to_draft
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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-co
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
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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
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 measu
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
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 b
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 sh
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
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
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
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
___
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
@@ -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.
-
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 interes
@@ -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.
-
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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listin
@@ -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 g
@@ -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.
-
@@ -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 g
@@ -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.
-
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
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
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
> >
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
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 ambiguou
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`
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,
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-commit
hahnjo wrote:
Hi @emaxx-google, any updates on the second minimization run?
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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::
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 pr
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 loo
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.co
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-pro
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
> > > >
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)](htt
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.
>
> > >
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 neede
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
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/
ilya-biryukov wrote:
> @ilya-biryukov, would you mind giving this PR a test on your infrastructure
> and if it works maybe share some performance results?
Sure, let me try kicking it off. Note that our infrastructure is much better at
detecting the compilations timing out than providing proper
hahnjo wrote:
### Performance measurements with LLVM
I tested these patches for building LLVM itself with modules
(`LLVM_ENABLE_MODULES=ON`). To work around
https://github.com/llvm/llvm-project/issues/130795, I apply
https://github.com/llvm/llvm-project/pull/131354 before building Clang. In
vgvassilev wrote:
@ilya-biryukov, would you mind giving this PR a test on your infrastructure and
if it works maybe share some performance results?
https://github.com/llvm/llvm-project/pull/133057
___
cfe-commits mailing list
cfe-commits@lists.llvm.or
hahnjo 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
https://github.com/root-project/root/pull/17722#issuecomment-2706555950 he
prefers a s
https://github.com/hahnjo created
https://github.com/llvm/llvm-project/pull/133057
* 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`
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Jonas Hahnfeld (hahnjo)
Changes
* 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::p
50 matches
Mail list logo
