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