[PATCH] D20268: Resolution-based LTO API.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:94
@@ +93,3 @@
+class InputFile {
+  friend LTO;
+  InputFile(std::unique_ptr<object::IRObjectFile> Obj) : Obj(std::move(Obj)) {}
----------------
mehdi_amini wrote:
> I missed how this friend allows access `Obj` earlier, and it is not great. It is used to create a dependency on having a Module available and on having "GlobalValue *", while we want to be able to make all the resolution without loading IR.
> Anytime LTO is accessing an IR construct through an InputFile, there's something that looks like a boundary violation.
> 
> As soon as we have a proper symbol table, InputFile should not even expose access to a module or IR at all. 
I agree with you, but until we have a symbol table, there needs to be some way for LTO to access IR entities in the input file.

Added a FIXME for the removal of these friends.

================
Comment at: include/llvm/LTO/LTO.h:103
@@ +102,3 @@
+  static ErrorOr<std::unique_ptr<InputFile>> create(MemoryBufferRef Object,
+                                                    LTO &Lto);
+
----------------
mehdi_amini wrote:
> > Let's not pretend that we can parse input files in a multithreaded way that allows them to be used later for linking until we can actually do it
> 
> I'm not sure what you mean: this is *already* what we do with libLTO:
> 
> The file is lazily loaded in its own context, this is cheap: the IR is not fully parsed but only the list of symbols. It is then reloaded separately, and non lazily, in the LTO context later for the purpose of LTO merging.
> If you have performance concern, we need to time this first. I can try to write a test tomorrow.
> 
> Right now this choice is penalizing ThinLTO that won't be able to load symbols in parallel and still have to load two times anyway.
> If you have performance concern, we need to time this first. I can try to write a test tomorrow.

Please let me know what you find. Bear in mind that you are the one advocating for a change here (the status quo in the only current client of this interface, the gold plugin, is a single context) so the burden is on you to justify it. If you wish to make a change, you are welcome to propose it separately from this patch.

================
Comment at: lib/LTO/LTO.cpp:280
@@ +279,3 @@
+Error LTO::addThinLto(std::unique_ptr<InputFile> Input,
+                      SmallPtrSet<GlobalValue *, 8> &Used,
+                      ArrayRef<SymbolResolution> Res) {
----------------
mehdi_amini wrote:
> > Because there may be no regular LTO module available as part of the link.
> 
> Client can just create one then...
> 
> ```
> Module M;
> Lto.add(M);
> // done
> ```
> 
> > if some of the ThinLTO modules define common symbols then we need to make sure that one of the object files contains the resolved common symbols.
> 
> I don't know what you're referring to here?
> Client can just create one then...

Not as easily as that. Roughly:

```
if (/* there are no regular LTO objects, oh, looks like we'll need more API surface */) {
  LLVMContext Ctx;
  Module M(Ctx);
  M.setTargetTriple(??? /* yet more API surface */);
  // add common symbols
  std::string BC;
  raw_str_ostream OS(BC);
  WriteBitcodeToFile(M, OS);
  ErrorOr<unique_ptr<InputFile>> F = InputFile::create(BC);
  if (!F) { /* error handling */ }
  Lto.add(std::move(*F));
}
```

Seems much simpler to always have a module with index 0.

> I don't know what you're referring to here?

As I previously mentioned on this code review:

> the special handling for common symbols required in the gold plugin (search for addCommons).


https://reviews.llvm.org/D20268