akyrtzi added inline comments.
================
Comment at: llvm/include/llvm/Support/PGOOptions.h:18
#include "llvm/Support/Error.h"
+#include "llvm/Support/VirtualFileSystem.h"
----------------
steven_wu wrote:
> akyrtzi wrote:
> > akyrtzi wrote:
> > > 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.
> > > 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.
> >
> > `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?
> 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.
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;
```
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
