[PATCH] D97370: [clang][cli] Remove marshalling from Opt{In, Out}FFlag

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97370/new/ https://reviews.llvm.org/D97370

[PATCH] D96847: [clang][cli] Store additional optimization remarks info

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96847/new/ https://reviews.llvm.org/D96847

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

dexonsmith added reviewers: jansvoboda11, Bigcheese. dexonsmith added a subscriber: Bigcheese. dexonsmith added a comment. This looks much cleaner; thanks for the update! I'm not sure this has decided whether it's writing special case code for building a CompilerInvocation's module context hash,

[PATCH] D104106: [clang][deps] NFC: Stop using moved-from object

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104106/new/ https://reviews.llvm.org/D104106 __

[PATCH] D104104: [clang][deps] NFC: Handle `DependencyOutputOptions` only once

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104104/new/ https://reviews.llvm.org/D104104 __

[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with one suggestion inline. Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:194 CompilerInstance &I, Depe

[PATCH] D103461: [clang][deps] NFC: Preserve the original frontend action

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Okay, LGTM. (Sorry for the delay, I've been out.) In D103461#2793254 , @jansvoboda11 wrote: > My reason for the FIXME is that we could get ri

[PATCH] D104012: [clang][deps] Move stripping of diagnostic serialization from `clang-scan-deps` to `DependencyScanning` library

dexonsmith accepted this revision. dexonsmith added a comment. LGTM too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104012/new/ https://reviews.llvm.org/D104012 ___ cfe-commits mailing list cfe-commit

[PATCH] D103802: [clang][modules][pch] Allow loading PCH with different modules cache path

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103802/new/ https://reviews.llvm.org/D103802 __

[PATCH] D103526: [clang][deps] Handle modular dependencies present in PCH

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103526/new/ https://reviews.llvm.org/D103526 __

[PATCH] D103807: [clang][deps] Ensure deterministic order of TU '-fmodule-file=' arguments

dexonsmith added a comment. I'm not sure the performance problems with std::map will matter in practice here, but have you considered sorting before emission rather than relying on the data structure's iteration order? (It'd make it easy to switch to StringMap in the future.) Repository: rG

[PATCH] D101506: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rG3302af9d4c39: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101506

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

dexonsmith added a subscriber: arphaman. dexonsmith added a comment. In D103930#2820725 , @bruno wrote: > Thanks for working on this, comments inline. @vsapsai @jansvoboda11 > @dexonsmith any headermap related concerns on your side? @jansvoboda11, I thi

[PATCH] D104459: [clang][lex] Ensure minimizer output is never larger than input

dexonsmith added a comment. > This patch ensures the output of source minimizer is never larger than the > input. This property will be handy in a follow up patch that makes it > possible to pad the minimized output to the size of the original input. I suppose when I wrote first wrote this I wa

[PATCH] D104465: [clang][deps] Prevent PCH validation failures by padding minimized files

dexonsmith added a comment. > This patch fixes dependency scanning of a TU with prebuilt modular/PCH > dependencies. > > Such modules/PCH might have been built using non-minimized files, while > dependency scanner does use the minimized versions of source files. Implicit > build doesn't like th

[PATCH] D104462: [clang][lex] Add minimizer option to pad the output to the input size

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. I'm not sure D104465 (the motivation for this patch) is the right approach, but we can have that discussion over there.

[PATCH] D104350: [clang][AST] Make `getLocalOrImportedSubmoduleID` work with const `Module*`. NFC.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104350/new/ https://reviews.llvm.org/D104350 __

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

dexonsmith added inline comments. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:27 +#define FOO +// This include will fail if modules weren't used. The include name itself +// doesn't exist and relies on the header map to remap it to the real header. ---

[PATCH] D104484: [clang] Add cc1 option for dumping layout for all complete types

dexonsmith added inline comments. Comment at: clang/lib/AST/Decl.cpp:4585 + + ASTContext &Ctx=getASTContext(); + if (Ctx.getLangOpts().DumpRecordLayoutsComplete) { Nit: please add whitespace around `=`. (Have you run git-clang-format? That should fix this.)

[PATCH] D104459: [clang][lex] Ensure minimizer output is never larger than input

dexonsmith added a comment. Another thing to consider: a client that wants the minimized source to be "no bigger than" the original source can use the original source if it ends up growing. For example, https://reviews.llvm.org/D104462 could check the resulting size, and just return the origina

[PATCH] D104465: [clang][deps] Prevent PCH validation failures by padding minimized files

dexonsmith added a comment. In D104465#2825343 , @dexonsmith wrote: > A few other ideas: > > 1. Disable PCH validation when minimizing. (Is that less robust than your > current workaround? If so, can you explain why?) > 2. Use the original PCH header in

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

dexonsmith closed this revision. dexonsmith added a comment. I pushed this as 05d0f1a8ea012a6b7b8ea65893ec4121106444b5  last night, but I just realized I forgot to include the link to the review :(. Closing manually. Reposit

[PATCH] D104484: [clang] Add cc1 option for dumping layout for all complete types

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104484/new/ https://reviews.llvm.org/D104484 __

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

dexonsmith added a comment. Given how large this is, would it be reasonable to split this up a bit more? What I might do if this were my patch: get a review of the API change + the manual changes in one patch (assuming there aren't many manual changes), then land the remaining mechanical change

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, once @MaskRay is happy. I have a couple of minor comments inline too. (I also see that there are some clang-format suggestions in the unit tests; not sure any of them are actuall

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

dexonsmith added a comment. In D104819#2839315 , @mstorsjo wrote: > In D104819#2839263 , @dexonsmith > wrote: > >> Just be sure to clang-format when you do the mechanical changes in the >> follow up patches.) >

[PATCH] D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests

dexonsmith added a comment. In D107155#2916664 , @dblaikie wrote: > Hmm, don't we have some other substitution feature that avoids the need to > add the %? (like, I think LLVM's tests don't usually use the % for many tool > names - but they still make f

[PATCH] D95581: Frontend: Refactor compileModuleAndReadAST, NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc130300f8ba0: Frontend: Refactor compileModuleAndReadAST, NFC (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D95583: Frontend: Add -f{, no-}implicit-modules-uses-lock and -Rmodule-lock

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb714f73defc8: Frontend: Add -f{,no-}implicit-modules-uses-lock and -Rmodule-lock (authored by dexonsmith). Changed prior to commit: https://review

[PATCH] D107296: Treat inttypes.h as a built-in header

dexonsmith resigned from this revision. dexonsmith added reviewers: bruno, vsapsai. dexonsmith added subscribers: bruno, vsapsai. dexonsmith added a comment. @bruno or @vsapsai, any chance one of you knows whether this is correct? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D95583: Frontend: Add -f{, no-}implicit-modules-uses-lock and -Rmodule-lock

dexonsmith added inline comments. Comment at: clang/test/Modules/implicit-modules-use-lock.m:21 + +// CHECK-NO-LOCKS-NOT: remark: +// CHECK-LOCKS: remark: locking '{{.*}}.pcm' to build module 'X' [-Rmodule-lock] arichardson wrote: > jansvoboda11 wrote: > > dexon

[PATCH] D108215: [clang][deps] NFC: Move `ResourceDirectoryCache`

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. I don't think this should be reused. If I'm reading the code correctly, this is just a super heavyweight (but cached) call to `Driver::GetResourcesPath()`. It should probably

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

dexonsmith added inline comments. Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:289 + +bool MIRProfileLoaderPass::runOnMachineFunction(MachineFunction &MF) { + if (!MIRSampleLoader->isValid()) JDevlieghere wrote: > Why is this outside the `llvm` namespace?

[PATCH] D108366: [clang][deps] Deduce resource directory from the compiler path

dexonsmith added a comment. The patch seems mostly straightforward, but can you clarify whether there was a functionality change for clang-scan-deps? My reading of the code suggests not, because it was using ResourceDirectoryCache before and still will... in which case, can you walk me through

[PATCH] D105052: [clang][darwin] add support for Mac Catalyst availability

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, but since this hasn't been up for long maybe leave it open for a bit in case someone else has a comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105052/new/ https

[PATCH] D103461: [clang][deps] NFC: Preserve the original frontend action

dexonsmith added a comment. In D103461#2841918 , @bmahjour wrote: > @jansvoboda11 This change is causing the following LIT tests to fail on AIX: I think @jansvoboda11 is out this week (IIRC, back next week). A possible interim workaround would be to add

[PATCH] D105257: [clang][darwin] add support for remapping macOS availability to Mac Catalyst availability

dexonsmith added a comment. Could the DarwinSDKInfo changes be tested directly with C++ unit tests? Since this stuff is in Basic it'd be nice to test it separately from the driver. If so, maybe they could also mostly land in a separate prep commit (except the update to `clang::parseDarwinSDKInf

[PATCH] D105257: [clang][darwin] add support for remapping macOS availability to Mac Catalyst availability

dexonsmith added inline comments. Comment at: clang/lib/Sema/Sema.cpp:70 + if (!SDKInfo) +llvm::handleAllErrors(SDKInfo.takeError(), + [](const llvm::ErrorInfoBase &) { If you're not looking at the error payload, maybe yo

[PATCH] D103165: Threading: use independent llvm::thread implementation on Apple platforms to increase stack size

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. This LGTM, with some whitespace nits inline. In D103165#2790932 , @t.p.northover wrote: >> Also this way llvm::thread users that don't need f

[PATCH] D105695: [clang][tooling] Accept Clang invocations with "-fno-integrated-as"

dexonsmith added inline comments. Comment at: clang/lib/Tooling/Tooling.cpp:119-122 + if (isa(A)) { +ExternalAssembler = true; +break; + } Seems like this could (unexpectedly?) let through a command with multiple `-cc1`s (if I'm reading

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

dexonsmith added a comment. Does this need an upgrade for old bitcode that has serialized 32-bit types? If not, can you explain why not? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105491/new/ https://reviews.llvm.org/D105491 __

[PATCH] D105958: [clang][darwin] add support for version remapping to the Darwin SDK Info class

dexonsmith added a subscriber: aaron.ballman. dexonsmith added a comment. Thanks for splitting this out! It mostly looks good, just a few things inline. Comment at: clang/lib/Basic/DarwinSDKInfo.cpp:27-28 +return KV->getSecond(); + // If no exact entry found, try just the

[PATCH] D105257: [clang][darwin] add support for remapping macOS availability to Mac Catalyst availability

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, assuming @aaron.ballman is happy! (a couple of optional nits inline) Comment at: clang/lib/Basic/DarwinSDKInfo.cpp:28 + // If no exact entry found, try just th

[PATCH] D105960: [clang][driver][darwin] Add driver support for Mac Catalyst

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM! (Maybe leave a couple of days in case others have comments.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105960/new/ https://rev

[PATCH] D105695: [clang][tooling] Accept Clang invocations with multiple cc1 jobs

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105695/new/ https://reviews.llvm.org/D105695 ___ cfe-commits mailing list cfe-

[PATCH] D112289: Support: Use sys::path::is_style_{posix,windows}() in a few places

dexonsmith updated this revision to Diff 383206. dexonsmith retitled this revision from "Support: Use sys::path::system_style() in a few places" to "Support: Use sys::path::is_style_{posix,windows}() in a few places". dexonsmith edited the summary of this revision. dexonsmith added reviewers: mst

[PATCH] D112289: Support: Use sys::path::is_style_{posix,windows}() in a few places

dexonsmith updated this revision to Diff 383207. dexonsmith added a comment. Undo a no-op change in the Path unit tests that I didn't intend to put here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112289/new/ https://reviews.llvm.org/D112289 Files: clang/include/clang/Basic/JsonSu

[PATCH] D112289: Support: Use sys::path::is_style_{posix,windows}() in a few places

dexonsmith updated this revision to Diff 383218. dexonsmith added a comment. Fix some backwards checks in FileManagerTest.cpp. (check-clang check-llvm passes now.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112289/new/ https://reviews.llvm.org/D112289 Files: clang/include/clang/B

[PATCH] D112289: Support: Use sys::path::is_style_{posix,windows}() in a few places

dexonsmith updated this revision to Diff 383446. dexonsmith edited the summary of this revision. dexonsmith added a comment. Squashed in the changes from https://reviews.llvm.org/D112786. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112289/new/ https://reviews.llvm.org/D112289 Files:

[PATCH] D112289: Support: Use sys::path::is_style_{posix,windows}() in a few places

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG99023627010b: Support: Use sys::path::is_style_{posix,windows}() in a few places (authored by dexonsmith). Repository: rG LLVM Github Monorepo CH

[PATCH] D112289: Support: Use sys::path::is_style_{posix,windows}() in a few places

dexonsmith added a comment. In D112289#3097959 , @thakis wrote: > Looks like this breaks tests on windows: > http://45.33.8.238/win/47971/step_7.txt > > Please take a look and revert for now if it takes a while to fix. Should be fixed in 9091df5fad52ab6

[PATCH] D112915: WIP: [clang][modules] Granular tracking of includes

dexonsmith added a comment. Just a few comments on implementation details. The only high-level piece to call out is that I wonder if `NumIncludes` could/should be simplified (semantically) to a Boolean in a prep commit. Comment at: clang/include/clang/Lex/Preprocessor.h:720-7

[PATCH] D112915: [clang][modules] Track number of includes per submodule

dexonsmith added a comment. Implementation looks a lot cleaner! I'd still like to drop NumIncludes first if possible because I think it'll be easier to reason about without this extra layer of complexity. Also, that'd mitigate the potential regression in `.pcm` size. (Note: I'd be more comfort

[PATCH] D97850: Fix PCM read from ModuleCache for ext4 filesystem

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Agreed that the right fix is to remove/replace the inode-based cache, but it's not safe to just drop it. Consider the following filesystem layout: /dir/file.h /dir/symlin

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

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

[PATCH] D113761: [clang][modules] NFC: Factor out path-based submodule lookup

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113761/new/ https://reviews.llvm.org/D113761 ___

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

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D109128#3097060 , @keith wrote: > @dexonsmith turns out the test I was adding is broken on main today too. If > it's alright with you I wil

[PATCH] D114093: [clang][lex] Refactor check for the first file include

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM with one nit inline. Comment at: clang/include/clang/Lex/Lexer.h:140-141 + /// True if this is the first time we're lexing the input file. + bool IsFirstTime

[PATCH] D114095: [clang][lex][modules] Move number of includes from HeaderFileInfo to Preprocessor

dexonsmith added a comment. I'm not sure this commit stands on its own, since it introduces a (temporary) regression in `.pcm` size; I suggest squashing with the follow-up, https://reviews.llvm.org/D114096. Comment at: clang/include/clang/Lex/HeaderSearch.h:55 + // TODO: Whe

[PATCH] D114096: [clang][lex][modules] Stop tracking number of includes

dexonsmith added a comment. I have a couple of nits inline, but otherwise LGTM if you squash with https://reviews.llvm.org/D114095. Comment at: clang/include/clang/Lex/Preprocessor.h:1230-1232 /// Increment the count for the number of times the specified FileEntry has //

[PATCH] D114092: [clang][lex] NFC: Remove unused HeaderFileInfo member

dexonsmith accepted this revision. dexonsmith added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114092/new/ https://reviews.llvm.org/D114092 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D112915: [clang][modules] Track number of includes per submodule

dexonsmith added inline comments. Comment at: clang/include/clang/Serialization/ModuleFile.h:399 + /// include information. + llvm::StringMap> SubmoduleIncludedFiles; + jansvoboda11 wrote: > dexonsmith wrote: > > Each StringMapEntry is going to have a pretty bi

[PATCH] D114120: [clang] Remove CLANG_ROUND_TRIP_CC1_ARGS and always roundtrip in +assert builds

dexonsmith added a comment. SGTM; I'll let @jansvoboda11 confirm though. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114120/new/ https://reviews.llvm.org/D114120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D114163: Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible.

dexonsmith added a reviewer: arphaman. dexonsmith added a subscriber: arphaman. dexonsmith added a comment. This revision now requires review to proceed. @arphaman , can you take a look? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114163/new/ htt

[PATCH] D114095: [clang][lex] Include tracking: simplify and move to preprocessor

dexonsmith added a comment. This LGTM by the way (not sure if that was already clear, since you abandoned a different review than I anticipated when squashing, and I'd "accepted" the other one); also see my suggestion to move the call to getFileInfo inside of markIncluded (might be error prone

[PATCH] D114966: [clang][deps] Split filesystem caches

dexonsmith added a comment. Thanks for working on this; seems like a great start. At a high-level: - We should check overhead. It'd be good to benchmark scanning LLVM with `clang-scan-deps` before and after this change. - The locking is getting harder to track, since the acquisition and release

[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. In D114968#3167519 , @jansvoboda11 wrote: > Assuming the filesystem doesn't change during dependency scanning, this > change keeps

[PATCH] D106876: Remove non-affecting module maps from PCM files.

dexonsmith added a comment. As an end goal, it seems strictly better for build systems / distribution / artifact storage / caching if the output PCM has been canonicalized as-if the module maps weren't found, rather than tracking both which module maps were discovered and which to ignore. Ideal

[PATCH] D106876: Remove non-affecting module maps from PCM files.

dexonsmith added a comment. In D106876#3869247 , @dexonsmith wrote: > A further (more involved) approach would be to separate module maps into a > separate SourceManager, so that their source locations don't affect other > input files. Then only module

[PATCH] D106876: Remove non-affecting module maps from PCM files.

dexonsmith added a comment. In D106876#3869447 , @jansvoboda11 wrote: > I agree that avoiding serializing non-affecting input files is the better > approach. Your code is more or less what I had in mind, thanks for sketching > it out! > The number of i

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3888387 , @jansvoboda11 wrote: > I tried optimizing this patch a bit. Instead of creating compact data > structure and using binary search to find the preceding non-affecting file, I > now store the adjustment inf

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added inline comments. Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32 + +// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o dexonsmi

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3893001 , @jansvoboda11 wrote: > I tried implementing your suggestion (merging ranges of adjacent > non-affecting files and avoiding `FileID` lookups), but the numbers from > `-ftime-trace` are very noisy. I got m

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3870991 , @hans wrote: > One thought, which I'm not sure is relevant, is that this is only observable > for headers which are included more than once, which is rare because normally > there are include guards (or p

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3898849 , @hans wrote: > Relatedly, we ran into a problem with `clang-cl /showIncludes` not including > all files in its output when they're linked: > https://github.com/llvm/llvm-project/issues/58726 Interestingl

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

dexonsmith added inline comments. Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170 + SCCProxy operator*() const { assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!"); return CurrentSCC; } + SCCProxy operator->() const { return operator*(); } -

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

dexonsmith added a comment. In D130303#3724247 , @rnk wrote: > Pinging alternative reviewer +@dexonsmith for a libclang API addition Looks reasonable to me -- this only changes behaviour of the existing API when there was corruption before -- but if the

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

dexonsmith added a subscriber: arphaman. dexonsmith added inline comments. Comment at: clang/test/FixIt/format.m:195-208 - NSLog(@"%C", 0x260300); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'int'}} - // CHECK-NOT: fix

[PATCH] D95502: WIP: Frontend: Adopt llvm::vfs::OutputBackend in CompilerInstance

dexonsmith abandoned this revision. dexonsmith added a comment. Herald added a project: All. This is superseded by https://reviews.llvm.org/D133509. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95502/new/ https://reviews.llvm.org/D95502 ___

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827040 , @aaron.ballman wrote: > In D134456#3819185 , @rnk wrote: > >> In D134456#3819042 , >> @aaron.ballman wrote: >> >>> Alter

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827263 , @aaron.ballman wrote: > In D134456#3827185 , @dexonsmith > wrote: > >> This safety scenario sounds like it could differ within a file. Is a flag >> really the ri

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827263 , @aaron.ballman wrote: > My worry is that parallel attributes will be used as: > > #if __has_cpp_attribute(clang::likely_but_honor_this_one) > #define LIKELY [[clang::likely_but_honor_this_one]] > #el

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827332 , @dexonsmith wrote: > In D134456#3827263 , @aaron.ballman > wrote: > >> My worry is that parallel attributes will be used as: >> >> #if __has_cpp_attribute(clang

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827377 , @aaron.ballman wrote: > In D134456#3827332 , @dexonsmith > wrote: > >> In D134456#3827263 , >> @aaron.ballman wrote: >>

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827478 , @aaron.ballman wrote: > In D134456#3827410 , @dexonsmith > wrote: > >> In D134456#3827377 , >> @aaron.ballman wrote: >>

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

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

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3839671 , @goncharov wrote: > That change might be problematic for content addressing storages. E.g. > clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as > clang/test/Driver/header{0,1,3,4}.h

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3840257 , @goncharov wrote: > In D135220#3840177 , @dexonsmith > wrote: > >> In D135220#3839671 , @goncharov >> wrote: >> >>> That

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3840257 , @goncharov wrote: > It's possbile to make it deterministic by making headers unique though. Also, this is the first time you've mentioned non-determinism. Is this non-deterministic? Repository: rG LLV

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dexonsmith added a comment. I would think we could convert every assert(0) to either `llvm::report_fatal_error` (guaranteed trap) or `llvm_unreachable()` (trap or optimize, depending on CMAKE configuration). The C API usage checks seem like good candidates for the former. Also, not sure if eve

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3862058 , @hans wrote: > The build system folks replied saying they're not using symlinks, but hard > links for compiler inputs. Dropping the `-s` flag in the repro above > demonstrates this. I think this only hap

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3900051 , @jansvoboda11 wrote: >> - Is there a change in cycles/instructions when the module cache is hot? >> (presumably the common case) > > I didn't notice this (but didn't look for it specifically). How could t

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3900593 , @jansvoboda11 wrote: > Ah, I forgot to mention this. Building the modules is now only 0.2% slower > and importing them 1.2% faster (compared to PCMs with all input files > serialized). Awesome. All upsi

[PATCH] D137215: [clang] NFC: Extract lower-level SourceManager functions

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137215/new/ https://reviews.llvm.org/D137215 __

[PATCH] D137214: [clang][modules] NFCI: Scaffolding for serialization of adjusted SourceManager offsets

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137214/new/ https://reviews.llvm.org/D137214 __

[PATCH] D137211: [clang][modules] NFCI: Unify FileID writing/reading

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137211/new/ https://reviews.llvm.org/D137211 __

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, although I there's format-is-probably-compatible-version as a courtesy for tooling, does that need a bump here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D137216: [clang][modules] NFCI: Avoid unnecessary serialization logic for non-affecting files

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137216/new/ https://reviews.llvm.org/D137216 __

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with one suggestion for the test inline. Comment at: clang/test/Modules/add-remove-irrelevant-module-map.m:22 +// Build modules with the non-affecting "a/module

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

dexonsmith added a comment. > Consider making the FileEntryRef changes here the default -- it doesn't make > sense to me that FileEntryRef == or DenseMap would match FileEntry pointer > semantics instead of filename semantics. Yeah, that was something I added, and I agree it's unfortunate / har

<    5   6   7   8   9   10   11   12   13   14   >