[PATCH] D110452: [modules] Fix tracking ObjCInterfaceType decl when there are multiple definitions.

vsapsai updated this revision to Diff 381313. vsapsai added a comment. Another rebase and pre-merge checks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110452/new/ https://reviews.llvm.org/D110452 Files: clang/include/clang/AST/Type.h clang/

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. Curiously, the results for the second project are unexpected and bizarre. | **File**| **Baseline (s)** | **Set Dedupe (s)** | **No external (s)** | **Set Dedupe (percentage of baseline)** | **No external (percentage of baseline)** | | Project2. File1 | 0.3785821

[PATCH] D110452: [modules] Fix tracking ObjCInterfaceType decl when there are multiple definitions.

This revision was automatically updated to reflect the committed changes. Closed by commit rGd9eca3320a4d: [modules] Fix tracking ObjCInterfaceType decl when there are multiple… (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110

[PATCH] D110453: [modules] Update visibility for merged ObjCInterfaceDecl definitions.

vsapsai updated this revision to Diff 381383. vsapsai added a comment. Rebase to trigger pre-merge checks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110453/new/ https://reviews.llvm.org/D110453 Files: clang/lib/Serialization/ASTReaderDecl.cp

[PATCH] D110453: [modules] Update visibility for merged ObjCInterfaceDecl definitions.

This revision was automatically updated to reflect the committed changes. Closed by commit rG048d2c76efcd: [modules] Update visibility for merged ObjCInterfaceDecl definitions. (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1104

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3079455 , @rmaz wrote: > So given these numbers are we good to go ahead with set dedupe approach? I'd rather get an opinion on this from other reviewers. It's not purely a numbers game, there can be other reasons to p

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. We are targeting the use case where in impl.m we have @import ImmediateDep1; @import ImmediateDep2; ... @import ImmediateDepN; and each of ImmediateDep has `@import SharedDep;`. For simplicity we'll consider that we are working with a single selector as each of

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. Raw data with some additional visualization is available at https://observablehq.com/@vsapsai/method_pool-performance-comparison Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109632/new/ https://reviews.llvm.org/D109632 _

[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

vsapsai added a comment. Not super happy with the complexity `DiagnosedError` introduces, want to consider alternatives first. Comment at: clang/lib/Serialization/ASTReader.cpp:3762 +return llvm::createStringError(std::errc::illegal_byte_sequence, +

[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

vsapsai added a comment. `DiagnosticError` looks like a good fit for the task at hand, so it is worth to try it. Though I don't know if it would end up in the end convoluted or OK. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108268/new/ https://

[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

vsapsai added a comment. In D108268#2961547 , @bnbarham wrote: > Unless we also change `DiagnosticEngine` it doesn't look like this is a > viable solution. The `PartialDiagnostic` can't be emitted straight to > `Diags`, since there may already be a diag

[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. Allowing multiple PartialDiagnostic to be in-flight/delayed is a nice goal but it's not clear if it's worth it. Also I'm not sure it is worth investing into making delayed diagnostic easier

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai added a comment. In D104344#2969178 , @jansvoboda11 wrote: > If my understanding is correct, this patch only changes how we track > `HeaderFileInfo::{isImport,NumIncludes}` and makes sure to store the > necessary info into AST files. Right? Ye

[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

This revision was automatically updated to reflect the committed changes. Closed by commit rGa4a5c00b53d0: [Modules] Change result of reading AST block to llvm::Error instead (authored by bnbarham, committed by vsapsai). Changed prior to commit: https://reviews.llvm.org/D108268?vs=368798&id=369

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106994/new/ https://reviews.llvm.org/D106994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

This revision was automatically updated to reflect the committed changes. Closed by commit rG93764ff6e200: [modules] Fix miscompilation when using two RecordDecl definitions with the… (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

vsapsai added a comment. Thanks for the review! Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3328-3331 // If there's no definition yet, then DC's definition is added by an update // record, but we've not yet loaded that update record. In this case, we //

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:1671 unsigned KeyLen = key.Filename.size() + 1 + 8 + 8; - unsigned DataLen = 1 + 2 + 4 + 4; + unsigned DataLen = 1 + 4 + 4 + 4; + for (auto ModuleIncluder : Data.HFI.ModuleInclu

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai updated this revision to Diff 370117. vsapsai added a comment. Clarify some comments; rebase for a fresh pre-commit testing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org/D104344 Files: clang/include/

[PATCH] D104351: [HeaderSearch] Use `isImport` only for imported headers and not for `#pragma once`.

This revision was automatically updated to reflect the committed changes. Closed by commit rG64ebf313a7e4: [HeaderSearch] Use `isImport` only for imported headers and not for `#pragma… (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D104344: [modules] Track how headers are included by different submodules.

vsapsai added a comment. That's a good point. Let me check how we track macros, I haven't thought about that approach. And I haven't considered using `Preprocessor::SubmoduleState`, was too excited `HeaderSearch::ShouldEnterIncludeFile` works correctly with the updated data.

[PATCH] D104344: [modules] Track how headers are included by different submodules.

vsapsai planned changes to this revision. vsapsai added a comment. Investigate less heavy-weight approaches similar to those used for tracking macros from different submodules. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://revi

[PATCH] D109128: [VFS] Use original path when falling back to external FS

vsapsai added a comment. Was looking at an issue caused by over-eager RedirectingFileSystem path canonicalization and this patch fixes it. The repro we have is more complicated (involves 3 modules), so I don't think it is worth including with this change. Comment at: llvm/lib

[PATCH] D109128: [VFS] Use original path when falling back to external FS

vsapsai added a comment. In D109128#2999846 , @dexonsmith wrote: > In D109128#2997588 , @JDevlieghere > wrote: > >> Keith and I discussed this offline. My suggestion was to do the following: >> >> 1. Check the o

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. I'm a little bit confused here. Where is the speed-up coming from? Is it because ObjCMethodList for `init` not being serialized? I'm asking because right now I don't see how `seen` helps with that. My current understanding is that `Sema::addMethodToGlobalList` is too ex

<    6   7   8   9   10   11