[PATCH] D151398: [clang] Make `FileEntryRef::getDir()` return the as-requested `DirectoryEntryRef`

rmaz accepted this revision. rmaz added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Basic/FileEntry.h:124 +/// Directory the file was found in. OptionalDirectoryEntryRef Dir; If this is always set

[PATCH] D147561: [clang] don't serialize MODULE_DIRECTORY with ModuleFileHomeIsCwd

This revision was automatically updated to reflect the committed changes. Closed by commit rG8ee5029b225b: [clang] dont serialize MODULE_DIRECTORY with ModuleFileHomeIsCwd (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147561/new/

[PATCH] D147561: [clang] don't serialize MODULE_DIRECTORY with ModuleFileHomeIsCwd

rmaz created this revision. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix a bug in the MODULE_DIRECTORY serialization logic that would cause MODULE_DIRECTORY to be serialized when

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz updated this revision to Diff 497309. rmaz added a comment. remove `header_file_size()` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143414/new/ https://reviews.llvm.org/D143414 Files: clang/include/clang/Basic/FileManager.h

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz added a comment. In D143414#4110461 , @benlangmuir wrote: >> This should allow the path serialization of input files to use the paths >> used when looking up a file entry, instead of the last reference. > > Isn't this at odds with not having the

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz updated this revision to Diff 495512. rmaz edited the summary of this revision. rmaz added a comment. Use FileEntryRef, rename method Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143414/new/ https://reviews.llvm.org/D143414 Files:

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz updated this revision to Diff 495259. rmaz added a comment. Don't return virtual file entries Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143414/new/ https://reviews.llvm.org/D143414 Files: clang/include/clang/Basic/FileManager.h

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz added inline comments. Comment at: clang/include/clang/Basic/FileManager.h:316 + void getSeenFileEntries( + SmallVectorImpl ) + const; jansvoboda11 wrote: > Since we're already modifying the two only users of this function, maybe we > could use

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz updated this revision to Diff 495191. rmaz added a comment. Sort by UIDs, then Name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143414/new/ https://reviews.llvm.org/D143414 Files: clang/include/clang/Basic/FileManager.h

[PATCH] D142780: [clang] NFCI: Use FileEntryRef in FileManager's UID mapping

rmaz added a comment. In D142780#4087273 , @jansvoboda11 wrote: > Sounds like the easies way out is serializing all `FileEntryRef` objects we > know in deterministic order. How about something like https://reviews.llvm.org/D143414 ? Repository: rG

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

rmaz created this revision. Herald added a subscriber: mgrang. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Refactor `FileManager::GetUniqueIDMapping` to populate an array of

[PATCH] D142780: [clang] NFCI: Use FileEntryRef in FileManager's UID mapping

rmaz added a comment. In D142780#4087270 , @benlangmuir wrote: > If compiling a single pcm accesses multiple hard links with the same UID, > then it would not be possible to use the set of UIDs to get the "right path". > At best we could make it get

[PATCH] D142780: [clang] NFCI: Use FileEntryRef in FileManager's UID mapping

rmaz added a comment. In D142780#4087236 , @benlangmuir wrote: > I think if we want to change this to FileEntryRef it needs to be > deterministic which ref you get. I think this might be the root of the problem we are seeing: depending on build

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

rmaz added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:663 Result->IsInferred = true; - Result->addTopHeader(File); + Result->addTopHeader(File->getLastRef()); jansvoboda11 wrote: > rmaz wrote: > > jansvoboda11 wrote: > > > How

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

rmaz marked an inline comment as done. rmaz added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:663 Result->IsInferred = true; - Result->addTopHeader(File); + Result->addTopHeader(File->getLastRef()); jansvoboda11 wrote: > How much

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

rmaz updated this revision to Diff 492829. rmaz added a comment. Use FileEntryRef for AddTopHeader() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142724/new/ https://reviews.llvm.org/D142724 Files: clang/include/clang/Basic/Module.h

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

rmaz created this revision. Herald added a subscriber: arphaman. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Refactor a Module's TopHeaders to use FileEntryRef. This will keep the paths serialized in a

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG75fbb5d2238f: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

rmaz updated this revision to Diff 490624. rmaz added a comment. rebase on last successful build: https://buildkite.com/llvm-project/llvm-main/builds/6356 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142113/new/ https://reviews.llvm.org/D142113

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

rmaz updated this revision to Diff 490568. rmaz added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142113/new/ https://reviews.llvm.org/D142113 Files: clang/include/clang/Basic/Module.h

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

rmaz abandoned this revision. rmaz added a comment. Its going to be less work to refactor TopheaderNames to use FileEntryRef, so went that way instead. First step is here: https://reviews.llvm.org/D142113 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

rmaz created this revision. Herald added a subscriber: ChuanqiXu. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Refactor the `Module::Header` class to use an `OptionalFileEntryRef` instead of a

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

rmaz added a comment. In D142028#4062798 , @akyrtzi wrote: > Is it reasonable to at least have an alternative implementation for it (e.g. > from the module headers find the headers that were not included from other > headers), not sure if it'd be

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

rmaz added a comment. @akyrtzi it looks like you added this field a while back. Do you know if there are still consumers for the the libclang API, do we need to keep this field around? The alternative is to refactor the `TopHeaders` set to use `FileEntryRef` so that we can keep track of the

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

rmaz created this revision. Herald added a subscriber: arphaman. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We are seeing issues with path serialization of the `SUBMODULE_TOPHEADER` field when the

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz added a comment. In D133586#3831618 , @vsapsai wrote: > How correct is it to access `isConst`, `isVolatile`, `isRestrict` for > `FunctionNoProtoType`? Yes, we can provide some default value but I'm curious > if accessing that default value is

[PATCH] D134911: [clang][DebugInfo] Respect fmodule-file-home-is-cwd in skeleton CUs for clang modules

rmaz accepted this revision. rmaz added a comment. This revision is now accepted and ready to land. Nice, LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134911/new/ https://reviews.llvm.org/D134911

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1f451a8bd6f3: [clang] initialize type qualifiers for FunctionNoProtoType (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 462898. rmaz added a comment. remove else case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 462518. rmaz added a comment. Remove unnecessary friend class Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 462483. rmaz added a comment. Return default Qualifiers for FunctionNoProtoType Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 462275. rmaz added a comment. add -std=c89 to test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz added inline comments. Comment at: clang/include/clang/AST/Type.h:3947 + Info) { +FunctionTypeBits.FastTypeQuals = 0; + } aaron.ballman wrote: > It seems a bit odd to me that we only want to initialize one member of the > bits and

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz added a comment. In D133586#3809475 , @aaron.ballman wrote: > This is why I'm wondering how we're hitting this problem in the first place. > C++ shouldn't be able to create a function without a prototype so why does > the ODR hash matter (do we

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 462248. rmaz added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h clang/unittests/AST/DeclTest.cpp Index:

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 461912. rmaz added a comment. Specify target and language for test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 461689. rmaz edited the summary of this revision. rmaz added a comment. Add unit test for type qualifiers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files:

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

rmaz added a comment. In D133586#3781536 , @rtrieu wrote: > In D133586#3781468 , @rmaz wrote: > >> In D133586#3781381 , @rtrieu wrote: >> >>> Do you have a test case for

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 459884. rmaz added a comment. zero out qual types in constructor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/include/clang/AST/Type.h Index:

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rmaz planned changes to this revision. rmaz added a comment. I think it is probably safer to zero out the FastTypeQuals instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586

[PATCH] D133611: [clang] sort additional module maps when serializing

This revision was automatically updated to reflect the committed changes. Closed by commit rG3c1b42347b3a: [clang] sort additional module maps when serializing (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133611/new/

[PATCH] D133611: [clang] sort additional module maps when serializing

rmaz updated this revision to Diff 459492. rmaz added a comment. clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133611/new/ https://reviews.llvm.org/D133611 Files: clang/lib/Serialization/ASTWriter.cpp Index:

[PATCH] D133611: [clang] sort additional module maps when serializing

rmaz updated this revision to Diff 459467. rmaz added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133611/new/ https://reviews.llvm.org/D133611 Files: clang/lib/Serialization/ASTWriter.cpp Index:

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rmaz added a comment. In D133586#3781381 , @rtrieu wrote: > Do you have a test case for this? Was struggling to think of a good one. How about a test that repeatedly generates a pcm for a func decl with no params and checks if the DECL_FUNCTION record

[PATCH] D133611: [clang] sort additional module maps when serializing

rmaz created this revision. Herald added a subscriber: mgrang. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Sort additional module maps when serializing pcm files. This ensures the `MODULE_MAP_FILE`

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rmaz updated this revision to Diff 459108. rmaz added a comment. Move code to VisitFunctionProtoType instead of branch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133586/new/ https://reviews.llvm.org/D133586 Files: clang/lib/AST/ODRHash.cpp

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

rmaz created this revision. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. When calculating the ODR hash of a `FunctionNoProtoType` do not include qualifiers derived from `FastTypeQuals`. These are only

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

rmaz added a comment. In D130710#3688338 , @akyrtzi wrote: > In D130710#3685436 , @benlangmuir > wrote: > >> Is the functionality provided by `ORIGINAL_PCH_DIR` still useful for making >> it possible to move a

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

This revision was automatically updated to reflect the committed changes. Closed by commit rGee51e9795a31: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124946/new/

[PATCH] D124938: [clang] serialize SUBMODULE_TOPHEADER relative to BaseDirectory

This revision was automatically updated to reflect the committed changes. Closed by commit rGf11056943e56: [clang] serialize SUBMODULE_TOPHEADER relative to BaseDirectory (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124938/new/

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

This revision was automatically updated to reflect the committed changes. Closed by commit rG646e502de0d8: [clang] add -fmodule-file-home-is-cwd (authored by rmaz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124874/new/

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

rmaz added a comment. In D124946#3500454 , @urnathan wrote: > Looks good to me, but perhaps leave it a few days for others to comment (my > familiarity with this code is low). I do know people want relocatable > outputs though. Are we good to go with

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

rmaz updated this revision to Diff 428483. rmaz added a comment. Refactor branch into existing case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124874/new/ https://reviews.llvm.org/D124874 Files: clang/include/clang/Driver/Options.td

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

rmaz added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:1231-1236 +SmallString<128> BaseDir(CWD->getName()); +cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir); +BaseDirectory.assign(BaseDir.begin(), BaseDir.end()); + }

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

rmaz updated this revision to Diff 427731. rmaz added a comment. fix for missing temp dir in test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124946/new/ https://reviews.llvm.org/D124946 Files: clang/lib/Serialization/ASTReader.cpp

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

rmaz updated this revision to Diff 427065. rmaz added a comment. fix windows paths attempt #2 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124874/new/ https://reviews.llvm.org/D124874 Files: clang/include/clang/Driver/Options.td

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

rmaz created this revision. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This diff changes the serialization of the `ORIGINAL_PCH_DIR` entry in module files to be serialized relative to the module's

[PATCH] D124938: [clang] serialize SUBMODULE_TOPHEADER relative to BaseDirectory

rmaz created this revision. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This diff changes the serialization of the `SUBMODULE_TOPHEADER` entry in module files to be serialized relative to the module's

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

rmaz updated this revision to Diff 427034. rmaz added a comment. Use regex for path separators in test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124874/new/ https://reviews.llvm.org/D124874 Files: clang/include/clang/Driver/Options.td

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

rmaz created this revision. Herald added a project: All. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This diff adds a new frontend flag `-fmodule-file-home-is-cwd`. The behavior of this flag is similar to

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz updated this revision to Diff 400983. rmaz added a comment. Rebase on stable commit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116174/new/ https://reviews.llvm.org/D116174 Files: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz updated this revision to Diff 400894. rmaz added a comment. Windows test fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116174/new/ https://reviews.llvm.org/D116174 Files: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz updated this revision to Diff 400102. rmaz added a comment. Update VirtualFileSystemTest to validate relative root paths. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116174/new/ https://reviews.llvm.org/D116174 Files:

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz updated this revision to Diff 48. rmaz added a comment. Add comment to describe relative root path behaviour Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116174/new/ https://reviews.llvm.org/D116174 Files:

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz added a comment. In D116174#3242087 , @benlangmuir wrote: > Each VFS may have its own working directory, so it could be surprising if we > use the OS working directory instead of that. This is complicated by the > fact the VFS working directory

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz updated this revision to Diff 395896. rmaz added a comment. clang-format changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116174/new/ https://reviews.llvm.org/D116174 Files: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz updated this revision to Diff 395893. rmaz added a comment. lint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116174/new/ https://reviews.llvm.org/D116174 Files: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml

[PATCH] D116174: [clang] support relative roots to vfs overlays

rmaz created this revision. Herald added subscribers: dexonsmith, hiraditya. rmaz requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This diff adds support for relative roots to VFS overlays. The directory root will be made

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

rmaz added a comment. > I don't recall any issues on my last benchmark, but i'll run the patch across > all of our modular files and see if anything comes up. I ran the patch against ~1500 sources here and there were no unexpected warnings or failures. Repository: rG LLVM Github Monorepo

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

rmaz added a comment. In D109632#3088479 , @vsapsai wrote: > Code-wise I'm not aware of any remaining issues. Still need to update the > commit message and to re-run the clang test suite. But you can totally use > the patch for testing. I plan to

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

rmaz abandoned this revision. rmaz added a comment. @vsapsai i'll abandon this diff then, thanks for your extensive feedback on the approach. Is D110123 shippable already, or are there some more corner cases to cover? Repository: rG LLVM Github Monorepo

[PATCH] D110092: [clang][NFC] encapsulate global method list in GlobalMethodPool

rmaz abandoned this revision. rmaz added a comment. Abandoning in favor of D110123 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110092/new/ https://reviews.llvm.org/D110092

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

rmaz added a comment. In D109632#3085187 , @dexonsmith wrote: > But another benefit of not double-storing transitively imported methods is > that it makes the PCMs more independent, tacking slightly closer to > "ImmediateDep1.pcm" being reproducible

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

rmaz added a comment. I am seeing the clean build times behaving the same as the populated ones, just slower: | *File*| *Baseline (s)* | *Set Dedupe* | *No External* | | IGMFVC.mm | 230| 194 | 195 | | So given these numbers are we good to go ahead with set

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

rmaz added a comment. In D109632#3076586 , @vsapsai wrote: > Methodology: clear a modules cache, compile a file once to pre-populate the > cache, compile file 8 times and measure elapsed time, take the time average. This is the same approach I used,

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

rmaz added a comment. In D109632#3062608 , @vsapsai wrote: > I can be wrong but I like writing less data as it can result in savings for > more projects. For example, if compile time is dominated by method > deserialization and not by

[PATCH] D111457: [clang][test] Add lit helper for windows paths

rmaz added inline comments. Comment at: clang/test/lit.cfg.py:60 +if platform.system() == 'Windows': +root_sep = 'C:\\' +else: keith wrote: > rmaz wrote: > > `c:\`? > Do you mean lowercase or a single slash? I see ~2x the number of uppercase vs > lowercase

[PATCH] D111457: [clang][test] Add lit helper for windows paths

rmaz added inline comments. Comment at: clang/test/lit.cfg.py:60 +if platform.system() == 'Windows': +root_sep = 'C:\\' +else: `c:\`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111457/new/

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

rmaz added a comment. In D109632#3046976 , @vsapsai wrote: > These are interesting results. I'm curious to measure the time spent in > `ASTReader::ReadMethodPool`. Updated numbers from today, looks like we added a lot more modular deps since last

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

rmaz added a comment. In D109632#3037501 , @vsapsai wrote: > My assumption was that all dependent modules are in memory at this point. And > we visit transitive modules only once, so I don't expect it to be a big > performance hit (though I can be

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

rmaz added a comment. In D109632#3032381 , @vsapsai wrote: > What's interesting, I was able to trigger more diagnostic. Specifically, I > got warnings about `length` ambiguity because in NSStatusItem it is CGFloat >

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

rmaz added a comment. Avoiding serializing external methods in `ASTMethodPoolTrait` is working successfully, although the times are not as good as using set deduplication: | **file** | **baseline** | **set dedupe** | **no external** | | Total | 149.68 | 92.97 | 109.83

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

rmaz added a comment. In D109632#3022209 , @vsapsai wrote: > That patch looks correct, I was experimenting with exactly the same local > change. Have you tried D110123 on your > original build? In my testing with

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

rmaz added a comment. @vsapsai just checking on where we are with this one. Is the current plan to investigate an approach only serializing the methods declared in each module, or are we good to go ahead with the set de-duplication approach? I tried profiling with D110123

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

rmaz added a comment. > What folks are thinking about writing less in METHOD_POOL? I prefer the idea of it, but I think the `ReadMethodPoolVisitor` also has to be changed for this to work. When it finds a selector in a module it will return true, which causes the search to stop descending into

[PATCH] D110092: [clang][NFC] encapsulate global method list in GlobalMethodPool

rmaz updated this revision to Diff 373675. rmaz added a comment. remove unnecessary empty check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110092/new/ https://reviews.llvm.org/D110092 Files: clang/include/clang/Sema/Sema.h

[PATCH] D110092: [clang][NFC] encapsulate global method list in GlobalMethodPool

rmaz created this revision. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This change moves the `addMethodToGlobalList` function to be a private member function of the `GlobalMethodPool` class. This is a preparatory step to allow

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

rmaz added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1434-1436 +bool addMethod(ObjCMethodDecl *Method) { + return AddedMethods.insert(Method).second; +} rmaz wrote: > dexonsmith wrote: > > Hmm, I was imagining that the set would

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

rmaz added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1434-1436 +bool addMethod(ObjCMethodDecl *Method) { + return AddedMethods.insert(Method).second; +} dexonsmith wrote: > Hmm, I was imagining that the set would be more

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

rmaz added a comment. In D109632#3005512 , @vsapsai wrote: > Thanks for the explanation! I'm still curious to reproduce the problem > locally and have created a test case generator > https://gist.github.com/vsapsai/f9d3603dde95eebd23248da4d7b4f5ec It

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

rmaz updated this revision to Diff 373072. rmaz added a comment. Update with single DenseSet per GlobalMethodPool Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109632/new/ https://reviews.llvm.org/D109632 Files: clang/include/clang/Sema/Sema.h

[PATCH] D109898: [clang][NFC] refactor GlobalMethodPool to encapsulate its map

rmaz updated this revision to Diff 372999. rmaz added a comment. - keep `std::pair`, but move to `GlobalMethodPool::Lists` - add `const` - `val` -> `&` - move comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109898/new/

[PATCH] D109898: [clang][NFC] refactor GlobalMethodPool to encapsulate its map

rmaz created this revision. rmaz added reviewers: dexonsmith, manmanren, vsapsai. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This refactor changes the GlobalMethodPool to a class that contains the DenseMap of methods. This is to

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

rmaz added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1423-1426 +ObjCMethodList InstanceMethods; +ObjCMethodList FactoryMethods; +llvm::DenseSet AddedInstanceMethods; +llvm::DenseSet AddedFactoryMethods; dexonsmith wrote: > Do

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

rmaz updated this revision to Diff 372598. rmaz added a comment. update to fix lint warnings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109632/new/ https://reviews.llvm.org/D109632 Files: clang/include/clang/Sema/Sema.h

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

rmaz updated this revision to Diff 372597. rmaz added a comment. Updated to use a GlobalMethods struct which contains the global method lists as well as the sets used to de-duplicate added methods. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

rmaz added a comment. In D109632#3000469 , @vsapsai wrote: > 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

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

rmaz added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:8188-8190 + for (auto *L = L; L = L->getNext()) { +seen.insert(L->getMethod()); + } dexonsmith wrote: > I find quadratic algorithms a bit scary, even when current benchmarks

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

rmaz added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:8194 +if (seen.insert(M).second) { + S.addMethodToGlobalList(, M); +} manmanren wrote: > Does it make sense to check for duplication inside addMethodToGlobalList, as >

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

rmaz created this revision. rmaz requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This diff will de-duplicate methods read from AST files before inserting them in to a global method pool list. When reading ObjCMethodDecl from AST files we

  1   2   >