[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Argyrios Kyrtzidis via Phabricator via llvm-commits]

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