https://github.com/ilya-biryukov approved this pull request.
LGTM
https://github.com/llvm/llvm-project/pull/133997
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commi
ilya-biryukov wrote:
> So the meaning of stacked PR in this series of patches is pretty
> questionable. So I feel it is better to merge these PRs into a single commit
> so that it is easier to be cherry-picked and reverted in my experience
+100, this is definitely the right call.
... and I wis
ilya-biryukov wrote:
Once again, thanks for bearing with us and addressing all the issues.
The latest version seems both correct and does not cause performance
regressions.
Let's land this!
PS please note that the resolution of our compile-time profiling instruments is
not that great, we might
https://github.com/ilya-biryukov approved this pull request.
https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
ilya-biryukov wrote:
Thanks for bearing with us. The latest round of correctness testing came out
positive. We have had one sleeper issue (the venerable "inline function
undefined") pop up in the leaf of the build graph, but we managed to work
around it.
@alexfh is still running the benchmark
ilya-biryukov wrote:
> @vgvassilev @ilya-biryukov @alexfh If I read correctly, the only blocker
> issue is the above reported performance issue. And I tried to split partial
> specialization from the full specialization table to avoid merging tables
> again and again. Could you please take ano
ilya-biryukov wrote:
> > > Sorry, could you provide the hash id for the commit that avoid the
> > > warning?
> >
> >
> > I tried running this on head and unfortunately it reproduces on head as
> > well :( So this looks like a sleeper issue which now also gets triggered by
> > this PR in a no
ilya-biryukov wrote:
We've hit only one correctness issue that we don't yet have a reproducer for,
but it gives this error:
```shell
Eigen/.../plugins/CommonCwiseBinaryOps.inc:47:29: error: inline function
'Eigen::operator*' is not defined [-Werror,-Wundefined-inline]
```
I'll be on vacation fo
ilya-biryukov wrote:
There were some merge conflicts with main, I've managed to resolve them to
this: https://github.com/ilya-biryukov/llvm-project/tree/pr83237_merged
I am now trying to bootstrap the compiler with that version, but it would be
useful if someone rebased this PR against main, i
ilya-biryukov wrote:
Sorry for loosing track of this, we'll run another round of tests and get back
to you.
https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/
ilya-biryukov wrote:
I had to replicate what our build was doing very thoroughly, the exact PCMs
each build action receives were important.
Here's the repro:
[maps.tgz](https://github.com/user-attachments/files/16992332/maps.tgz). Just
like last time, it works without the patch and starts fail
ilya-biryukov wrote:
I got to a small reproducer that only uses STL, but it only produces an error
in our environment and if I try it with this patch, the error goes away.
I am probably missing something subtle, will dig deeper tomorrow.
Sorry for another delay.
https://github.com/llvm/llvm-pr
ilya-biryukov wrote:
Just wanted to give a short update that I'm still on it, but it takes longer
than I initially anticipated.
I've got entangled into a few spaghetti-like dependencies and untangling them
takes a bit of time... I should be much closer now, hope to share something
tomorrow.
h
ilya-biryukov wrote:
We did manage to run another round of testing and it fails, with somewhat
familiar module-related issues:
```cpp
[third_party/absl/container/internal/compressed_tuple.h:250]:24: error:
'absl::container_internal::CompressedTuple>>::get' from module
'//third_party/absl/cont
ilya-biryukov wrote:
Thanks for fixing the problem.
@alexfh and another person who was running these investigations before is on
vacation until next week. I will ask if someone else can do this for them, but
I wouldn't be surprised that it's involved enough that we may need to wait
until next
ilya-biryukov wrote:
Here is the reproducer. Like before, builds at head and fails with the patch
applied. After unpacking the archive just run
```
$ CLANGPP= ./build.sh
```
Note the comment in `build.sh` about the system libc++ required for it (libc++
17 worked, 15 and 16 did not).
[container
ilya-biryukov wrote:
Sorry for disappearing, I had a holiday, vacation and some unplanned
interruptions over the last week and the start of this week. I have made really
good progress and the amount of code that I need to dig through is quite
manageable.
I should have a repro for you this wee
ilya-biryukov wrote:
We've hit an error during testing:
```cpp
In module '...stl_cc_library':
.../src/libcxx/include/__memory/allocator.h:128:60: error:
'std::allocator>::deallocate' from
module '...stl_cc_library./cxx17/vector' is not present in definition of
'std::allocator>' provided earlie
ilya-biryukov wrote:
> In the meantime, could you rebase the patch on top of current ToT?
I thought that it should not have any merge conflicts and it turned out to be
almost true, the only conflict I encountered was in `CMakeLists.txt`, feel free
to use this squashed and rebased commit for te
ilya-biryukov wrote:
> @ilya-biryukov, could we have another try of this PR on your end?
Sorry for missing this, we will rerun the testing and get back to you.
https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-b
@@ -7392,27 +7388,28 @@ QualType ASTReader::GetType(TypeID ID) {
return TypesLoaded[Index].withFastQualifiers(FastQuals);
}
-QualType ASTReader::getLocalType(ModuleFile &F, unsigned LocalID) {
+QualType ASTReader::getLocalType(ModuleFile &F, TypeID LocalID) {
---
@@ -70,38 +71,53 @@ using DeclID = DeclIDBase::DeclID;
/// An ID number that refers to a type in an AST file.
///
-/// The ID of a type is partitioned into two parts: the lower
+/// The ID of a type is partitioned into three parts:
+/// - the lower
/// three bits are used to
@@ -12,44 +12,44 @@
// RUN: -fmodule-file=Module=%t/Module.pcm -emit-llvm -o - | FileCheck
%t/Object.cppm
// Test again with reduced BMI.
-// RUN: rm -rf %t
-// RUN: mkdir -p %t
-// RUN: split-file %s %t
+// RUNX: rm -rf %t
ilya-biryukov wrote:
are these
@@ -70,38 +71,53 @@ using DeclID = DeclIDBase::DeclID;
/// An ID number that refers to a type in an AST file.
///
-/// The ID of a type is partitioned into two parts: the lower
+/// The ID of a type is partitioned into three parts:
+/// - the lower
/// three bits are used to
@@ -6659,13 +6655,22 @@ void ASTWriter::MacroRead(serialization::MacroID ID,
MacroInfo *MI) {
}
void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
- // Always take the highest-numbered type index. This copes with an
interesting
+ // Always take the type index that comes i
@@ -6659,13 +6655,22 @@ void ASTWriter::MacroRead(serialization::MacroID ID,
MacroInfo *MI) {
}
void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
- // Always take the highest-numbered type index. This copes with an
interesting
+ // Always take the type index that comes i
@@ -6659,13 +6655,22 @@ void ASTWriter::MacroRead(serialization::MacroID ID,
MacroInfo *MI) {
}
void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
- // Always take the highest-numbered type index. This copes with an
interesting
+ // Always take the type index that comes i
@@ -3262,17 +3262,18 @@ void ASTWriter::WritePragmaDiagnosticMappings(const
DiagnosticsEngine &Diag,
/// Write the representation of a type to the AST stream.
void ASTWriter::WriteType(QualType T) {
TypeIdx &IdxRef = TypeIdxs[T];
- if (IdxRef.getIndex() == 0) // we haven't
@@ -7650,6 +7647,16 @@ ModuleFile *ASTReader::getOwningModuleFile(GlobalDeclID
ID) const {
return &getModuleManager()[ModuleFileIndex - 1];
}
+ModuleFile *ASTReader::getOwningModuleFile(TypeID ID) const {
+ if (ID < NUM_PREDEF_TYPE_IDS)
+return nullptr;
+
+ uint64_t M
@@ -70,38 +71,53 @@ using DeclID = DeclIDBase::DeclID;
/// An ID number that refers to a type in an AST file.
///
-/// The ID of a type is partitioned into two parts: the lower
+/// The ID of a type is partitioned into three parts:
+/// - the lower
/// three bits are used to
@@ -7100,14 +7084,25 @@ TypeSourceInfo *ASTRecordReader::readTypeSourceInfo() {
return TInfo;
}
+std::pair
+ASTReader::translateTypeIDToIndex(serialization::TypeID ID) const {
+ unsigned Index =
+ (ID & llvm::maskTrailingOnes(32)) >> Qualifiers::FastWidth;
+
+ ModuleF
https://github.com/ilya-biryukov commented:
Thanks for the change! I have done a round of review and left a few suggestion
and also have a bunch of questions. I am sorry if some of them may look too
obvious, I did try to dig into the code where I could, but Clang's
serialization is complicated
https://github.com/ilya-biryukov edited
https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
ilya-biryukov wrote:
I am happy to take a look, but will run out of time for it today. I will
definitely do it tomorrow, though.
https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
ht
https://github.com/ilya-biryukov approved this pull request.
Went through the changes one more time and they definitely LGTM.
See the two small comments left from my side about documentation and release
notes, I think they're worth fixing before landing this. Otherwise, let's ship
it!
https://
@@ -9304,7 +9299,8 @@ TemplateName
ASTContext::getAssumedTemplateName(DeclarationName Name) const {
TemplateName ASTContext::getQualifiedTemplateName(NestedNameSpecifier *NNS,
bool TemplateKeyword,
@@ -9304,7 +9299,8 @@ TemplateName
ASTContext::getAssumedTemplateName(DeclarationName Name) const {
TemplateName ASTContext::getQualifiedTemplateName(NestedNameSpecifier *NNS,
bool TemplateKeyword,
@@ -804,6 +804,8 @@ Bug Fixes to AST Handling
- Clang now properly preserves ``FoundDecls`` within a ``ConceptReference``.
(#GH82628)
- The presence of the ``typename`` keyword is now stored in
``TemplateTemplateParmDecl``.
- Fixed malformed AST generated for anonymous union
https://github.com/ilya-biryukov commented:
The change makes handling of template names more uniform, which overall looks
like a great change.
However, we are changing the contract of `QualifiedTemplateName` (it can have a
null qualifier now) and this seems both counter-intuitive from the API
https://github.com/ilya-biryukov edited
https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
ilya-biryukov wrote:
After endless hours and recompilations, I finally have this to share.
It could probably be smaller, but I decided to share this as soon as it crashed
outside of our monorepo.
[module.tgz](https://github.com/llvm/llvm-project/files/15223606/module.tgz)
^^ If you run build.s
ilya-biryukov wrote:
> Can you export all files into a standalone reproducer? I might be able to
> reduce an example.
Not really, this is why it's taking so long. Our infrastructure in that space
is lacking, the issue is that the root case is not in one compilation step, but
rather in some of
ilya-biryukov wrote:
Sorry, still struggling to get a small repro. The build graphs we have are
quite large, unfortunately.
Did any of the stack traces or error message I posted help to find certain
problems? Or is there no hope until we get a smaller repro?
https://github.com/llvm/llvm-projec
ilya-biryukov wrote:
We have hit quite a few issues when trying this out. I have spent a day trying
to reduce to a small repro that I can share, but haven't not fully succeeded
yet, I will continue tomorrow.
Still wanted to share the error descriptions in case that would allow to make
progress
ilya-biryukov wrote:
Thanks for fixing it quickly, we'll try the new patch and get back with the
results.
https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cg
ilya-biryukov wrote:
Sorry for the delay, I had an emergency and took a week off.
We have hit some form of OOM (an infinite loop or some exponential computation
allocating memory?) on the following small example (ready as a test case, also
in [this
commit](https://github.com/ilya-biryukov/llv
ilya-biryukov wrote:
Btw, if I don't respond timely to these requests and you need an urgent
reaction, feel free to ping me via email at ibiryu...@google.com.
We heavily use Clang header modules internally at Google and we are really
interested in helping to discover and prevent the module bugs
ilya-biryukov wrote:
We'll pick this up, I'll get back with the testing results.
(Sorry for the delayed response)
https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm
48 matches
Mail list logo
