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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
