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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/COFF/SymbolTable.cpp:40
   log("Reading " + toString(file));
-  file->parse();
+  if (file->lazy) {
+    if (auto *f = dyn_cast<BitcodeFile>(file))
----------------
aganea wrote:
> 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?
lld/ELF needs `thinLTOCreateEmptyIndexFiles` because Bazel's distributed ThinLTO requires it (--thinlto-index-only). Bazel needs it because it's the build system that requires all specified input and output and the output needs to be unconditional.

OK, it seems that lld/COFF uses a different strategy (D64461) which avoids `lazyBitcodeFiles`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116434