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

[Steven Wu via Phabricator via llvm-commits]

steven_wu added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:164
           LLVMIRGenerationRefCount(0),
-          Gen(CreateLLVMCodeGen(Diags, InFile, std::move(FS), HeaderSearchOpts,
-                                PPOpts, CodeGenOpts, C, CoverageInfo)),
+          Gen(CreateLLVMCodeGen(Diags, InFile, FS, HeaderSearchOpts, PPOpts,
+                                CodeGenOpts, C, CoverageInfo)),
----------------
benlangmuir wrote:
> Wouldn't `move` be fine here since it's already copied to `this->FS`?
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.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1729
+  if (!Opts.ProfileInstrumentUsePath.empty()) {
+    auto FS = llvm::vfs::getRealFileSystem();
+    setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath, *FS, Diags);
----------------
akyrtzi wrote:
> 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.
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. 


================
Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:20
 #include "llvm/Support/Discriminator.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <memory>
----------------
akyrtzi wrote:
> Recommend to forward declare instead of including the header.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139052/new/

https://reviews.llvm.org/D139052