Page MenuHomePhabricator

[NFC][Profile] Access profile through VirtualFileSystem
ClosedPublic

Authored by steven_wu on Nov 30 2022, 3:33 PM.

Details

Summary

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.

Diff Detail

Event Timeline

steven_wu created this revision.Nov 30 2022, 3:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 3:33 PM
steven_wu requested review of this revision.Nov 30 2022, 3:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 30 2022, 3:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
akyrtzi added inline comments.Dec 2 2022, 3:34 PM
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?

benlangmuir added inline comments.
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?

steven_wu updated this revision to Diff 480990.Dec 7 2022, 11:13 AM
steven_wu marked 13 inline comments as done.

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.

benlangmuir accepted this revision.Dec 7 2022, 11:20 AM

All my comments are addressed, thanks!

This revision is now accepted and ready to land.Dec 7 2022, 11:20 AM
akyrtzi added inline comments.Dec 7 2022, 5:07 PM
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.

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

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?

steven_wu added inline comments.Dec 8 2022, 10:19 AM
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.

akyrtzi added inline comments.Dec 8 2022, 2:52 PM
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;
steven_wu updated this revision to Diff 487597.Mon, Jan 9, 4:16 PM

Address review feedback. Remove NFC from title.

steven_wu updated this revision to Diff 493724.Tue, Jan 31, 1:46 PM

Rebase to main.

akyrtzi accepted this revision.Tue, Jan 31, 3:41 PM
steven_wu updated this revision to Diff 493768.Tue, Jan 31, 4:16 PM

Try fix pre-merge build failure

This revision was automatically updated to reflect the committed changes.