akyrtzi added inline comments.
================ Comment at: clang/include/clang/CodeGen/BackendUtil.h:14 #include "llvm/IR/ModuleSummaryIndex.h" +#include "llvm/Support/VirtualFileSystem.h" #include <memory> ---------------- Recommend to forward declare instead of including the header. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1729 + if (!Opts.ProfileInstrumentUsePath.empty()) { + auto FS = llvm::vfs::getRealFileSystem(); + setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath, *FS, Diags); ---------------- Could we propagate the VFS that the `CompilerInvocation` is going to create here? Otherwise this seems like a hidden "landmine" that someone is bound to trip on later on. ================ Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:20 #include "llvm/Support/Discriminator.h" +#include "llvm/Support/VirtualFileSystem.h" #include <memory> ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:63 + + IntrusiveRefCntPtr<vfs::FileSystem> FS; }; ---------------- AFAICT this member is not used, you could remove it. ================ Comment at: llvm/include/llvm/Passes/PassBuilder.h:24 #include "llvm/Support/PGOOptions.h" +#include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:31 #include "llvm/Support/Error.h" +#include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/ProfileData/InstrProf.h:35 #include "llvm/Support/MathExtras.h" +#include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" ---------------- This is not used in this file, you could remove the include from here. ================ Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:29 #include "llvm/Support/SwapByteOrder.h" +#include "llvm/Support/VirtualFileSystem.h" #include <algorithm> ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:241 #include "llvm/Support/SymbolRemappingReader.h" +#include "llvm/Support/VirtualFileSystem.h" #include <cstdint> ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/Support/PGOOptions.h:18 #include "llvm/Support/Error.h" +#include "llvm/Support/VirtualFileSystem.h" ---------------- I'd suggest to consider moving the `PGOOptions` constructor out-of-line, along with ``` PGOOptions &operator=(const PGOOptions &O); PGOOptions(const PGOOptions&); ~PGOOptions(); ``` in order to forward-declare here and avoid including the header, avoiding the additional include dependencies for many cpp files. ================ Comment at: llvm/include/llvm/Support/PGOOptions.h:37 !PseudoProbeForProfiling)), - PseudoProbeForProfiling(PseudoProbeForProfiling) { + PseudoProbeForProfiling(PseudoProbeForProfiling), FS(FS) { // Note, we do allow ProfileFile.empty() for Action=IRUse LTO can ---------------- ================ Comment at: llvm/include/llvm/Transforms/IPO/SampleProfile.h:19 #include "llvm/Pass.h" +#include "llvm/Support/VirtualFileSystem.h" #include <string> ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/Transforms/IPO/SampleProfile.h:34 : ProfileFileName(File), ProfileRemappingFileName(RemappingFile), LTOPhase(LTOPhase) {} ---------------- ================ Comment at: llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h:20 #include "llvm/IR/PassManager.h" +#include "llvm/Support/VirtualFileSystem.h" #include <cstdint> ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:40 #include "llvm/Support/GenericDomTree.h" +#include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" ---------------- Recommend to forward declare instead of including the header. ================ Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:86 + IntrusiveRefCntPtr<vfs::FileSystem> FS) + : Filename(Name), RemappingFilename(RemapName), FS(FS) {} void dump() { Reader->dump(); } ---------------- ================ Comment at: llvm/lib/ProfileData/InstrProf.cpp:1228 CountSumOrPercent &Sum) -> Error { - auto ReaderOrErr = InstrProfReader::create(Filename); + auto FS = vfs::getRealFileSystem(); + auto ReaderOrErr = InstrProfReader::create(Filename, *FS); ---------------- Could you add a comment why this doesn't need a propagated VFS? ================ Comment at: llvm/lib/Target/X86/X86InsertPrefetch.cpp:162 LLVMContext &Ctx = M.getContext(); + auto FS = vfs::getRealFileSystem(); ErrorOr<std::unique_ptr<SampleProfileReader>> ReaderOrErr = ---------------- Could you add a comment why this doesn't need a propagated VFS? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139052/new/ https://reviews.llvm.org/D139052 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits