[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added a comment. Reverted in 7c2cb1448ad2d20e251db5e3ae4a0c84c12aa970 , relanded in 7b8cf98b4a9a2f5ea3667fdbf913a4f8952ed36a . Repository:

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:901 // across FileID lookups. if (!LocalSLocEntryTable[MiddleIndex].isExpansion()) LastFileIDLookup = Res; nickdesaulniers wrote: > oh, this case here shou

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:901 // across FileID lookups. if (!LocalSLocEntryTable[MiddleIndex].isExpansion()) LastFileIDLookup = Res; oh, this case here should have been removed, too

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added a comment. I appreciate you taking the time to review, and find additional optimizations! It's not populated yet, but IIUC, this should be the link with more test results: http://llvm-compile-time-tracker.com/compare.php?from=7cc5307c73caa72f22edd4208b175e3c36eec46e&to=df

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

This revision was automatically updated to reflect the committed changes. Closed by commit rGdffc1420451f: [clang][SourceManager] cache Macro Expansions (authored by nickdesaulniers). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80681/new/ https://

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM! Thanks for bearing with me on splitting the patches and such! Let's land it and see if this is going to regress any projects. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers marked 4 inline comments as done. nickdesaulniers added a comment. Cool, should be good for review. This change is now nice an isolated, in case we need to revert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80681/new/ https://r

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers updated this revision to Diff 273780. nickdesaulniers added a comment. - rebase on D82497 & D82498 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80681/new/ https:/

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added a comment. I've forked off 1. D82497 (`getFileIDLocal` optimizations) 2. D82498 (don't check `Invalid` for calls to `getLocalSLocEntry`) I agree those should be the less controversial parts of this diff.

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:967 if (isOffsetInFileID(FileID::get(-int(MiddleIndex) - 2), SLocOffset)) { FileID Res = FileID::get(-int(MiddleIndex) - 2); kadircet wrote: > my comments above for

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

kadircet added a comment. In D80681#2096886 , @nickdesaulniers wrote: > Heh, those were painstakingly written in markdown by hand, using > `CC="/usr/bin/time -v clang"` to test. The weren't statistically significant > (N=1), nor were they autogenerated

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added a comment. > I've seen quite nice benchmarks for kernel build times in the issues you've > mentioned it would be nice if you could back that claim with similar data(not > that I don't take your word for it, I just love tables and I am jealous that > kernel has some but we

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

kadircet added a comment. Also forgot to say, thanks a lot for taking your time to investigate this issue and coming up with a patch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80681/new/ https://reviews.llvm.org/D80681 _

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

kadircet added a comment. I am not the best person to review this, but dig a little bit into your issues and history of this file. Your diagnosis and fix seems reasonable, AFAICT the existing caching behavior was chosen to optimize files with many macro invocations that are expanding into relat

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added a comment. Bumping for review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80681/new/ https://reviews.llvm.org/D80681 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers added a comment. Bumping for review. Are there other trusted reviewers we could add the to review to help? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80681/new/ https://reviews.llvm.org/D80681 ___

[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

nickdesaulniers created this revision. nickdesaulniers added reviewers: rsmith, bkramer. Herald added subscribers: cfe-commits, jfb, kristof.beyls. Herald added a project: clang. A seemingly innocuous Linux kernel change [0] seemingly blew up our compile times by over 3x, as reported by @nathancha