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

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

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