[PATCH] D116434: [lld-link] Replace LazyObjFile with lazy ObjFile/BitcodeFile

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.
This revision is now accepted and ready to land.

LGTM, a few minor comments:



================
Comment at: lld/COFF/SymbolTable.cpp:585
   s->pendingArchiveLoad = true;
-  f->fetch();
+  assert(f->lazy);
+  f->lazy = false;
----------------
Is it more appropriate to move this assert at the top?


================
Comment at: lld/COFF/SymbolTable.cpp:587
+  f->lazy = false;
+  ctx.symtab.addFile(f);
 }
----------------
You can just write `addFile(f);`


================
Comment at: lld/COFF/SymbolTable.cpp:40
   log("Reading " + toString(file));
-  file->parse();
+  if (file->lazy) {
+    if (auto *f = dyn_cast<BitcodeFile>(file))
----------------
MaskRay wrote:
> aganea wrote:
> > I wonder if it's not better to leave types to do their own testing of `lazy` internally, in `Type::parse()`, and act accordingly (instead of doing it outside like here).
> > Same thing applies to pushing to the instances, shouldn't `parse()` do it internally? (but that could be done later)
> > We should be ending with just `file->parse();` here.
> The reason `parse` calling `parseLazy` may feel better is because the function is in `SymbolTable` (while it can be placed into `InputFile`, too), so `lazy` has to be public to be accessed here.
> 
> If the function is in `InputFile`, this function dispatching to `parse` and `parseLazy` will be superior in my view as we can avoid the slightly unnecessary abstraction that `parse` returns `parseLazy`...
> 
> ---
> 
> If PE/COFF needs the `lazyBitcodeFiles` as ELF does (Bazel distributed ThinLTO requirement), handling `lazy` in this function may be better, too.
> The reason `parse` calling `parseLazy` may feel better is because the function is in `SymbolTable` (while it can be placed into `InputFile`, too), so `lazy` has to be public to be accessed here.
> 
> If the function is in `InputFile`, this function dispatching to `parse` and `parseLazy` will be superior in my view as we can avoid the slightly unnecessary abstraction that `parse` returns `parseLazy`...
Fair enough, let's keep them apart.

> If PE/COFF needs the `lazyBitcodeFiles` as ELF does (Bazel distributed ThinLTO requirement),
Do you need that (ie.`thinLTOCreateEmptyIndexFiles()`) because you have to emit exactly the same number of index files as there are input objects? If LLD exits with no error, emits less index files than inputs, could the build system assume that only those files are to be distributed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116434