Make the access to profile data going through virtual file system so the
inputs can be remapped. In the context of the caching, it can make sure
we capture the inputs and provided an immutable input as profile data.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/CodeGen/BackendUtil.h | ||
---|---|---|
14 | Recommend to forward declare instead of including the header. | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
1728 | 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. | |
llvm/include/llvm/CodeGen/MIRSampleProfile.h | ||
21 | Recommend to forward declare instead of including the header. | |
67 | AFAICT this member is not used, you could remove it. | |
llvm/include/llvm/Passes/PassBuilder.h | ||
23 | Recommend to forward declare instead of including the header. | |
llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h | ||
31 | Recommend to forward declare instead of including the header. | |
llvm/include/llvm/ProfileData/InstrProf.h | ||
35 ↗ | (On Diff #479085) | This is not used in this file, you could remove the include from here. |
llvm/include/llvm/ProfileData/InstrProfReader.h | ||
31 | Recommend to forward declare instead of including the header. | |
llvm/include/llvm/ProfileData/SampleProfReader.h | ||
240 | Recommend to forward declare instead of including the header. | |
llvm/include/llvm/Support/PGOOptions.h | ||
19 | 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. | |
34–35 | ||
llvm/include/llvm/Transforms/IPO/SampleProfile.h | ||
20 | Recommend to forward declare instead of including the header. | |
35–36 | ||
llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h | ||
21 | Recommend to forward declare instead of including the header. | |
llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h | ||
41 | Recommend to forward declare instead of including the header. | |
89 | ||
llvm/lib/ProfileData/InstrProf.cpp | ||
1228–1231 | Could you add a comment why this doesn't need a propagated VFS? | |
llvm/lib/Target/X86/X86InsertPrefetch.cpp | ||
163 | Could you add a comment why this doesn't need a propagated VFS? |
clang/lib/CodeGen/BackendUtil.cpp | ||
---|---|---|
195 | Nit: std::move(VFS) to reduce refcounting. | |
clang/lib/CodeGen/CodeGenAction.cpp | ||
166 | Wouldn't move be fine here since it's already copied to this->FS? | |
llvm/lib/ProfileData/SampleProfReader.cpp | ||
1838 | Commented out code | |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
466 | move? |
Address review feedback.
clang/lib/CodeGen/CodeGenAction.cpp | ||
---|---|---|
166 | I think I confused myself for which FS is moved here. I renamed the parameter so it is clear move is applied to the FileSystem passed in. | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
1728 | Currently, Profile look up doesn't go through any VFS, so vfs overlay has no effect on profiles. Putting through VFS is changing behavior, even I think it is for good. It also has the potential to make code harder to read because creating VFS relies on a compiler invocation which we are creating here. Let me add a fixme here first. | |
llvm/include/llvm/CodeGen/MIRSampleProfile.h | ||
21 | The default parameter needs to instantiate here and pretty much all references to PGOOptions has Optional<PGOOptions> which requires including VFS. I will leave this one since PGOOptions.h is a rare header to include. |
clang/lib/Frontend/CompilerInvocation.cpp | ||
---|---|---|
1728 |
Your patch is already changing behavior; CodeGen will load profiles using clang's VFS, so vfs overlay will affect how profiles are loaded. The mismatch is that CodeGen will load the path using clang's VFS but setPGOUseInstrumentor will load it directly from real file-system, so they can be out-of-sync. On second look, setPGOUseInstrumentor seems to create a throw-away IndexedInstrProfReader just for reading a couple of settings to set some flags. This seems redundant, could we move the determination of these flags at the point where CodeGenModule is creating its own IndexedInstrProfReader and remove setPGOUseInstrumentor from CompilerInvocation::ParseCodeGenArgs entirely? | |
llvm/include/llvm/Support/PGOOptions.h | ||
19 |
PGOOptions.h is transitively included by many files, if I touch that header, and then compile clang, there are 225 files that need to be re-compiled. I thought that moving the PGOOptions members I mentioned out-of-line would be enough to avoid the need to include VirtualFileSystem.h, is this not the case? |
clang/lib/Frontend/CompilerInvocation.cpp | ||
---|---|---|
1728 | I will take a look. The current implementation is not very good as it just read it to determine the type of the profile to set the action. I agree the disconnect between two reads is bad. | |
llvm/include/llvm/Support/PGOOptions.h | ||
19 | I see. It is included in two different headers in the backend, both of them has Optional<PGOOptions>. During my experiment, to instantiate Optional<PGOOptions>, it needs full declaration for PGOOptions, thus FileSystem. I could be wrong but I don't see a way around that. |
llvm/include/llvm/Support/PGOOptions.h | ||
---|---|---|
19 | With this diff on top of your patch everything compiles fine, without a need to include the header in PGOOptions.h: diff --git a/llvm/include/llvm/Support/PGOOptions.h b/llvm/include/llvm/Support/PGOOptions.h --- a/llvm/include/llvm/Support/PGOOptions.h +++ b/llvm/include/llvm/Support/PGOOptions.h @@ -14,11 +14,15 @@ #ifndef LLVM_SUPPORT_PGOOPTIONS_H #define LLVM_SUPPORT_PGOOPTIONS_H +#include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/Support/Error.h" -#include "llvm/Support/VirtualFileSystem.h" namespace llvm { +namespace vfs { +class FileSystem; +} // namespace vfs + /// A struct capturing PGO tunables. struct PGOOptions { enum PGOAction { NoAction, IRInstr, IRUse, SampleUse }; @@ -28,35 +32,13 @@ struct PGOOptions { IntrusiveRefCntPtr<vfs::FileSystem> FS = nullptr, PGOAction Action = NoAction, CSPGOAction CSAction = NoCSAction, bool DebugInfoForProfiling = false, - bool PseudoProbeForProfiling = false) - : ProfileFile(ProfileFile), CSProfileGenFile(CSProfileGenFile), - ProfileRemappingFile(ProfileRemappingFile), Action(Action), - CSAction(CSAction), DebugInfoForProfiling(DebugInfoForProfiling || - (Action == SampleUse && - !PseudoProbeForProfiling)), - PseudoProbeForProfiling(PseudoProbeForProfiling), FS(std::move(FS)) { - // Note, we do allow ProfileFile.empty() for Action=IRUse LTO can - // callback with IRUse action without ProfileFile. - - // If there is a CSAction, PGOAction cannot be IRInstr or SampleUse. - assert(this->CSAction == NoCSAction || - (this->Action != IRInstr && this->Action != SampleUse)); - - // For CSIRInstr, CSProfileGenFile also needs to be nonempty. - assert(this->CSAction != CSIRInstr || !this->CSProfileGenFile.empty()); - - // If CSAction is CSIRUse, PGOAction needs to be IRUse as they share - // a profile. - assert(this->CSAction != CSIRUse || this->Action == IRUse); + bool PseudoProbeForProfiling = false); - // If neither Action nor CSAction, DebugInfoForProfiling or - // PseudoProbeForProfiling needs to be true. - assert(this->Action != NoAction || this->CSAction != NoCSAction || - this->DebugInfoForProfiling || this->PseudoProbeForProfiling); + // Out-of-line so we don't have to include `VirtualFileSystem.h` header. + PGOOptions(const PGOOptions&); + ~PGOOptions(); + PGOOptions &operator=(const PGOOptions &O); - // If we need to use the profile, the VFS cannot be nullptr. - assert(this->FS || !(this->Action == IRUse || this->CSAction == CSIRUse)); - } std::string ProfileFile; std::string CSProfileGenFile; std::string ProfileRemappingFile; diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt --- a/llvm/lib/Support/CMakeLists.txt +++ b/llvm/lib/Support/CMakeLists.txt @@ -201,6 +201,7 @@ add_llvm_component_library(LLVMSupport OptimizedStructLayout.cpp Optional.cpp Parallel.cpp + PGOOptions.cpp PluginLoader.cpp PrettyStackTrace.cpp RandomNumberGenerator.cpp diff --git a/llvm/lib/Support/PGOOptions.cpp b/llvm/lib/Support/PGOOptions.cpp new file mode 100644 --- /dev/null +++ b/llvm/lib/Support/PGOOptions.cpp @@ -0,0 +1,53 @@ +//===------ PGOOptions.cpp -- PGO option tunables --------------*- C++ -*--===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/Support/PGOOptions.h" +#include "llvm/Support/VirtualFileSystem.h" + +using namespace llvm; + +PGOOptions::PGOOptions(std::string ProfileFile, std::string CSProfileGenFile, + std::string ProfileRemappingFile, + IntrusiveRefCntPtr<vfs::FileSystem> FS, + PGOAction Action, CSPGOAction CSAction, + bool DebugInfoForProfiling, + bool PseudoProbeForProfiling) + : ProfileFile(ProfileFile), CSProfileGenFile(CSProfileGenFile), + ProfileRemappingFile(ProfileRemappingFile), Action(Action), + CSAction(CSAction), DebugInfoForProfiling(DebugInfoForProfiling || + (Action == SampleUse && + !PseudoProbeForProfiling)), + PseudoProbeForProfiling(PseudoProbeForProfiling), FS(std::move(FS)) { + // Note, we do allow ProfileFile.empty() for Action=IRUse LTO can + // callback with IRUse action without ProfileFile. + + // If there is a CSAction, PGOAction cannot be IRInstr or SampleUse. + assert(this->CSAction == NoCSAction || + (this->Action != IRInstr && this->Action != SampleUse)); + + // For CSIRInstr, CSProfileGenFile also needs to be nonempty. + assert(this->CSAction != CSIRInstr || !this->CSProfileGenFile.empty()); + + // If CSAction is CSIRUse, PGOAction needs to be IRUse as they share + // a profile. + assert(this->CSAction != CSIRUse || this->Action == IRUse); + + // If neither Action nor CSAction, DebugInfoForProfiling or + // PseudoProbeForProfiling needs to be true. + assert(this->Action != NoAction || this->CSAction != NoCSAction || + this->DebugInfoForProfiling || this->PseudoProbeForProfiling); + + // If we need to use the profile, the VFS cannot be nullptr. + assert(this->FS || !(this->Action == IRUse || this->CSAction == CSIRUse)); + } + +PGOOptions::PGOOptions(const PGOOptions&) = default; + +PGOOptions &PGOOptions::operator=(const PGOOptions &O) = default; + +PGOOptions::~PGOOptions() = default; |
Recommend to forward declare instead of including the header.