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

[Steven Wu via Phabricator via cfe-commits]

steven_wu 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);
----------------
akyrtzi wrote:
> 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?
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052