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

[Argyrios Kyrtzidis via Phabricator via llvm-commits]

akyrtzi added inline comments.


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


================
Comment at: llvm/include/llvm/Support/PGOOptions.h:18
 #include "llvm/Support/Error.h"
+#include "llvm/Support/VirtualFileSystem.h"
 
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052