[PATCH] D20268: Resolution-based LTO API.

[Mehdi AMINI via llvm-commits]

mehdi_amini 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)) {}
----------------
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. 

================
Comment at: include/llvm/LTO/LTO.h:103
@@ +102,3 @@
+  static ErrorOr<std::unique_ptr<InputFile>> create(MemoryBufferRef Object,
+                                                    LTO &Lto);
+
----------------
> 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.

================
Comment at: lib/LTO/LTO.cpp:280
@@ +279,3 @@
+Error LTO::addThinLto(std::unique_ptr<InputFile> Input,
+                      SmallPtrSet<GlobalValue *, 8> &Used,
+                      ArrayRef<SymbolResolution> Res) {
----------------
> 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?

================
Comment at: lib/LTO/LTOBackend.cpp:150
@@ +149,3 @@
+    return;
+
+  std::unique_ptr<TargetMachine> TM =
----------------
For ThinLTO at least, we don't support parallel codegen, so we don't need TM to be thread safe to be able to have only one TM created in `thinBackend()` and reused for both `opt` and `codegen`.

I even think that it should be already achievable by having:

- backend() and thinbackend() always creating the TM.
- opt() taking the TM as a parameter
- codegen() taking the TM as a parameter
- split_codegen() recreating the TM.

This way you only recreate an extra one in splitcodegen() when parallel codegen is enabled. Unless I missed something this is a net win.


https://reviews.llvm.org/D20268