ChuanqiXu9 wrote:
> Oh, I didn't realize you were eager to land this in Clang 19, thanks for
> sharing that. In that case, I think a specialiazed hash function for
> `GlobalDeclID` is indeed the way to go.
>
> I was also worried a little there are other performance implications of this
> chan
ilya-biryukov wrote:
Oh, I didn't realize you were eager to land this in Clang 19, thanks for
sharing that.
In that case, I think a specialiazed hash function for `GlobalDeclID` is indeed
the way to go.
I was also worried a little there are other performance implications of this
change that w
ChuanqiXu9 wrote:
> I got to the bottom of it. The problem is that our hash function for 64 bit
> ints is [not very
> good](https://github.com/llvm/llvm-project/blob/d5297b72aa32ad3a69563a1fcc61294282f0b379/llvm/include/llvm/ADT/DenseMapInfo.h#L140).
>
> It will have a lot of collision when lo
alexfh wrote:
Thank you for the analysis, Ilya!
Re: a possible resolution, I wonder why `llvm::hash_value` from
llvm/ADT/Hashing.h is not used in `DenseSet`? In any case, if this can't be
resolved quickly and needs a design discussion, it would be appropriate to
revert the patch first.
https
ilya-biryukov wrote:
I got to the bottom of it. The problem is that our hash function for 64 bit
ints is [not very
good](https://github.com/llvm/llvm-project/blob/d5297b72aa32ad3a69563a1fcc61294282f0b379/llvm/include/llvm/ADT/DenseMapInfo.h#L140).
It will have a lot of collision when lower 32
ilya-biryukov wrote:
While this looks like a useful optimization, I am still not sure why 2x more
data (is that a correct upper bound?) can lead to a 10x slowdown. I will try to
dig further to understand what's going on here.
https://github.com/llvm/llvm-project/pull/92083
ChuanqiXu9 wrote:
I sent https://github.com/llvm/llvm-project/pull/95506. It is a independent
patch which may mitigate the issue you met. @alexfh you can try this when you
have time.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mai
ChuanqiXu9 wrote:
Thanks for the profiling data. It narrows the scope a lot. But it makes me
confusing too. Since the scope is pretty narrow, we can do an analysis here:
https://github.com/llvm/llvm-project/blob/a7a1195f01037e5019f671c96ef4bca9af9bb9a7/clang/lib/Serialization/MultiOnDiskHashTab
ilya-biryukov wrote:
I only managed to confirm that the problem is that merging of individual PCMs
into a merged hash table has become much slower with this change.
I tried with #95348 too, but it didn't help.
I will continue my investigations and reductions tomorrow.
https://github.com/llvm/l
ilya-biryukov wrote:
In both cases we are condensing `1105` on-disk hash tables, but it took seconds
before and takes multiple minutes after the change.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llv
ilya-biryukov wrote:
I added a printf into `FindExternalVisibleDeclsByName` where it calls find on
`MultiOnDiskHashTable` to see what names have been causing this and I can see
that we stall on the first name we try to look up. The name `std` is the first
one and was previously processed almos
alexfh wrote:
To clarify a bit more: the host platform Clang runs on is x86-64 in all cases
described here by myself, @joanahalili, @asmok-g, @bgra8 and @ilya-biryukov.
What may be specific to aarch64 is a set of headers conditionally-included just
when compiling for ARM platforms.
As @ilya-b
ilya-biryukov wrote:
To clarify: we observe this regression on x86, not on Arm. @alexfh is now
trying to profile the compiler to locate what's going wrong and I will try to
poke at PCM files and come up with a reproducer.
However, the regressions is definitely real and will block our internal
ChuanqiXu9 wrote:
> The 10x increase in the time spent reading modules does look surprising and I
> would need to check if it's related to PCMs becoming larger or is localized
> to that particular compile. (Even if on a single example).
It should not related to the size of the PCMs otherwise
ilya-biryukov wrote:
The 10x increase in the time spent reading modules does look surprising and I
would need to check if it's related to PCMs becoming larger or is localized to
that particular compile. (Even if on a single example).
@ChuanqiXu9 do you expect alignment to have such a drastic e
ChuanqiXu9 wrote:
I sent https://github.com/llvm/llvm-project/pull/95348 for aligned related
change.
For size increase, the reason and the possible solution are clear. We should
store the declaration ID as two slots instead of one in the serialized format
so that we can utilize VBR6 format be
ChuanqiXu9 wrote:
Oh, the time regression is surprising to me. And I observed that @alexfh only
reports (compile performance) problems on AArch64 but @bgra8 reports (size
increase problems) on both ARM64 and X86_64.
@alexfh is it true?
So I **guess** the problem may come from the unaligned a
alexfh wrote:
So, what's happening here is a significant increase in the compilation time of
a pretty large source that has a large number of modular dependencies (using
Clang header modules rather than C++20 modules).
The times reported by `clang -ftime-report` for clang frontend change
dras
bgra8 wrote:
@ChuanqiXu9 the description of the change says that the maximum expected PCMs
size increase is 6.6%. In our testing at google we see noticed for some large
targets the total combined PCM size increase by more than 25% (both ARM64 and
x86_64).
https://github.com/llvm/llvm-project/
alexfh wrote:
As the architecture is concerned, it's AArch64, which is 64 bit.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexfh wrote:
The report is about compilation timeouts rather than runtime performance. The
code being compiled is outside the tree. We're working on a shareable
reproducer.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
ChuanqiXu9 wrote:
Oh, this is surprising. Are arm CPUs 32 bit? And how much do these tests get
slowed down?
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/
asmok-g wrote:
> just a heads up: we are seeing a number targets failing due to a compilation
> timeout caused by this commit.
More precisely, the timeouts are happening only on arm CPUs.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commit
joanahalili wrote:
just a heads up: we are seeing a number targets failing due to an compilation
timeout caused by this commit.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.
ChuanqiXu9 wrote:
Thanks for testing. I've resent
https://github.com/llvm/llvm-project/commit/5a0181f568e56e37df80d0f74eca4775776fa8cd.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lis
ChuanqiXu9 wrote:
Sadly it looks like the (refined) patch still breaks the alignment on 32 bit
machine. https://lab.llvm.org/buildbot/#/builders/174/builds/34307
I've reverted the patch and updated
https://github.com/llvm/llvm-project/pull/94603 to test it again.
https://github.com/llvm/llvm-
ChuanqiXu9 wrote:
Resent
https://github.com/llvm/llvm-project/commit/5c104879c1a98eeb845c03e7c45206bd48e88f0c
Thanks for testing it !
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://list
ChuanqiXu9 wrote:
> > I didn't recognize we may still have 32-bit machines
>
> I think 32 bit Arm are the last.
>
> > I'll revert it again.
>
> Sure, if you post the PR I can test the fix too. I know getting 32 bit
> machines can be tricky.
I sent https://github.com/llvm/llvm-project/pull/94
DavidSpickett wrote:
> Thanks. Reproduced. But it surprised me that I can't run the commands you
> mentioned, but I need to run:
> ./bin/lldb-dotest -p TestTemplateWithSameArg.py -G gmodules
Running the tests as modules is perhaps opt in, we have the ability to run them
with different debug in
ChuanqiXu9 wrote:
I've fixed the lldb failure and resent
https://github.com/llvm/llvm-project/commit/97c866f6c86456b3316006e6beff47e68a81c00a.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
ChuanqiXu9 wrote:
> Maybe you can run individual tests with lit but generally I use `lldb-dotest`
> instead:
>
> ```
> ./bin/lldb-dotest.py -p TestTemplateWithSameArg.py
> ```
>
> (you only need the filename)
Thanks. Reproduced. But it surprised me that I can't run the commands you
mentioned
DavidSpickett wrote:
Maybe you can run individual tests with lit but generally I use `lldb-dotest`
instead:
```
./bin/lldb-dotest.py -p TestTemplateWithSameArg.py
```
(you only need the filename)
https://github.com/llvm/llvm-project/pull/92083
___
cfe
ChuanqiXu9 wrote:
> Unfortunately this is still failing one test on the LLDB macOS CI:
> https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/5083/execution/node/97/log
>
> ```
> ==
> FAIL: test_duplicate_decls_g
Michael137 wrote:
> Oh, this is not wanted. It should be almost a NFC patch for users. I'll
> revert it.
Thanks!
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/ma
ChuanqiXu9 wrote:
Oh, this is not wanted. It should be almost a NFC patch for users. I'll revert
it.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listin
Michael137 wrote:
Unfortunately this is still failing one test on the macOS CI:
https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/5083/execution/node/97/log
```
==
FAIL: test_duplicate_decls_gmodules
(TestTem
ChuanqiXu9 wrote:
Thanks for reporting. I've reproduced them (including the lldb test) locally
and fixed them. I'll try to land it again to see what happens.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@list
@@ -111,6 +109,28 @@ void *Decl::operator new(std::size_t Size, const
ASTContext &Ctx,
return ::operator new(Size + Extra, Ctx);
}
+GlobalDeclID Decl::getGlobalID() const {
+ if (!isFromASTFile())
+return GlobalDeclID();
+ uint64_t ID = *((const uint64_t *)this - 1);
Michael137 wrote:
I.e., also broke the macOS LLDB buildbots:
https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/2637/execution/node/97/log/
```
Assertion failed: (!((*IDAddress) >> 48) && "We should only set the module ID
once"), function setOwningModuleID, file DeclBase.cpp, lin
hctim wrote:
Hi from the sanitizer buildbot mainter, thanks for the fast revert! For full
reproduction, you can use
https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild and
`buildbot_fast.sh`. But, you can reproduce it quicker by using
`-DCMAKE_C_FLAGS="-fsanitize=undefined"
DavidSpickett wrote:
This also broke an lldb test:
https://lab.llvm.org/buildbot/#/builders/96/builds/58449
`lldb-test: ../llvm-project/clang/lib/AST/DeclBase.cpp:132: void
clang::Decl::setOwningModuleID(unsigned int): Assertion `!((*IDAddress) >> 48)
&& "We should only set the module ID once
ChuanqiXu9 wrote:
I'll close it as there are some crashes.
---
The crash log:
```
** TEST 'Clang :: Modules/redecl-ivars.m' FAILED
Exit Code: 1
Command Output (stderr):
--
RUN: at line 2: rm -rf
/b/sanitizer-x86_64-linux-fast/build/llvm_build_asan_ubsan/tools/cla
https://github.com/ChuanqiXu9 closed
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ChuanqiXu9 wrote:
I'd like to land this since:
- I want to give more time testing this
- the design of the patch is pretty similar with the previous one
(https://github.com/llvm/llvm-project/pull/86912)
- the scale of the patch is not very big (100+ lines of code except new tests)
- I do think t
ChuanqiXu9 wrote:
Thanks for reviewing : )
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -111,6 +109,28 @@ void *Decl::operator new(std::size_t Size, const
ASTContext &Ctx,
return ::operator new(Size + Extra, Ctx);
}
+GlobalDeclID Decl::getGlobalID() const {
+ if (!isFromASTFile())
+return GlobalDeclID();
+ uint64_t ID = *((const uint64_t *)this - 1);
@@ -7802,20 +7800,31 @@ Decl *ASTReader::GetDecl(GlobalDeclID ID) {
LocalDeclID ASTReader::mapGlobalIDToModuleFileGlobalID(ModuleFile &M,
GlobalDeclID GlobalID) {
- DeclID ID = GlobalID.get();
- if (ID < NUM_PREDEF_DECL_
@@ -255,6 +255,12 @@ class DeclOffset {
}
};
+// The unaligned decl ID used in the Blobs of bistreams.
+using unalighed_decl_id_t =
ChuanqiXu9 wrote:
Nice catch! Done in
https://github.com/llvm/llvm-project/commit/e73e4951b20c70f24354e2a2820876c818dcaee3
@@ -124,6 +130,15 @@ class DeclIDBase {
bool isInvalid() const { return ID == PREDEF_DECL_NULL_ID; }
+ unsigned getModuleFileIndex() const { return ID >> 32; }
+
+ unsigned getLocalDeclIndex() const {
+// Implement it directly instead of calling `llvm::maskTrailingOne
https://github.com/ChuanqiXu9 updated
https://github.com/llvm/llvm-project/pull/92083
>From 8b7a650e128d6f2bfec9b0768169cf4aaa4af337 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu
Date: Fri, 10 May 2024 15:36:31 +0800
Subject: [PATCH] [serialization] no transitive decl change
---
clang/include/cla
https://github.com/jansvoboda11 commented:
This looks good conceptually, I left a couple of minor notes.
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/list
@@ -7802,20 +7800,31 @@ Decl *ASTReader::GetDecl(GlobalDeclID ID) {
LocalDeclID ASTReader::mapGlobalIDToModuleFileGlobalID(ModuleFile &M,
GlobalDeclID GlobalID) {
- DeclID ID = GlobalID.get();
- if (ID < NUM_PREDEF_DECL_
@@ -111,6 +109,28 @@ void *Decl::operator new(std::size_t Size, const
ASTContext &Ctx,
return ::operator new(Size + Extra, Ctx);
}
+GlobalDeclID Decl::getGlobalID() const {
+ if (!isFromASTFile())
+return GlobalDeclID();
+ uint64_t ID = *((const uint64_t *)this - 1);
@@ -255,6 +255,12 @@ class DeclOffset {
}
};
+// The unaligned decl ID used in the Blobs of bistreams.
+using unalighed_decl_id_t =
jansvoboda11 wrote:
I know this typo isn't introduced by your PR, but since your PR is touching it,
please fix it in a prep
@@ -124,6 +130,15 @@ class DeclIDBase {
bool isInvalid() const { return ID == PREDEF_DECL_NULL_ID; }
+ unsigned getModuleFileIndex() const { return ID >> 32; }
+
+ unsigned getLocalDeclIndex() const {
+// Implement it directly instead of calling `llvm::maskTrailingOne
ChuanqiXu9 wrote:
@jansvoboda11 @Bigcheese gentle ping
https://github.com/llvm/llvm-project/pull/92083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
56 matches
Mail list logo
