[PATCH] D20268: Resolution-based LTO API.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:103
@@ +102,3 @@
+  static ErrorOr<std::unique_ptr<InputFile>> create(MemoryBufferRef Object,
+                                                    LTO &Lto);
+
----------------
mehdi_amini wrote:
> IIUC the reason we need a context is because we can't get the information we want on an InputFile without a context for now.
> Since we have plan to fix this (and this is acknowledged in the FIXME), I'd rather remove the LTO param now and add a Context inside the `InputFile` itself. It'll:
> 1) solve any multithreading question about parsing input files.
> 2) allow to easily get rid of this context later.
> 
Sorry, but I'd prefer not to do this.

> solve any multithreading question about parsing input files.

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. As things stand, doing this will probably regress gold plugin performance as we will need to parse the input file an additional time.

> allow to easily get rid of this context later.

It doesn't seem like it will be particularly difficult to remove this argument in clients later, it would probably be on the order of a trivial mechanical change.

================
Comment at: include/llvm/LTO/LTO.h:321
@@ +320,3 @@
+  struct GlobalResolution {
+    /// The unmangled name of the global.
+    std::string IRName;
----------------
mehdi_amini wrote:
> At least, can we wrap them in struct to separate this clearly:
> 
> ```
> struct {
>   unsigned ParallelCodeGenParallelismLevel;
>   LTOLLVMContext Ctx;
>   std::unique_ptr<Module> CombinedModule;
>   IRMover Mover;
> } LTOState;
> 
>   // These fields are for ThinLTO.
> struct {
>   ThinBackend Backend;
>   ModuleSummaryIndex CombinedIndex;
>   MapVector<StringRef, MemoryBufferRef> ModuleMap;
>   DenseMap<GlobalValue::GUID, StringRef> PrevailingModuleForGUID;
> } ThinLTOState;
> ```
> 
> It'll make the code more clear on violation (ThinLTO code accessing LTO state or vice-versa)
Good idea, done.

================
Comment at: lib/LTO/LTO.cpp:280
@@ +279,3 @@
+  CombinedModule->setTargetTriple(M.getTargetTriple());
+  CombinedModule->setDataLayout(M.getDataLayout());
+
----------------
mehdi_amini wrote:
> This isn't clear to me "the client may want to add symbol definitions to it". Why doesn't the client add a regular LTO module if he needs to add its stuff?
Because there may be no regular LTO module available as part of the link. For the gold plugin for example 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. In the case where there are no regular LTO modules that can be an object file containing just the common symbols.

================
Comment at: lib/LTO/LTOBackend.cpp:149
@@ +148,3 @@
+  std::unique_ptr<TargetMachine> TM =
+      createTargetMachine(C, M.getTargetTriple(), TheTarget);
+  std::unique_ptr<raw_pwrite_stream> OS = AddStream(Task);
----------------
mehdi_amini wrote:
> Nothing critical but we just already created a TM for the optimization pipeline.
Sure, but it's a little simpler to create this where needed, especially when using parallel code gen. Eventually we might want to just create this once in backend() and thinBackend(), but that would require TargetMachine to be thread safe.

================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:34
@@ +33,3 @@
+static cl::list<std::string> SymbolResolutions(
+    "r",
+    cl::desc("Specify a symbol resolution: filename,symbolname,resolution\n"
----------------
mehdi_amini wrote:
> "r" is really not very explicit for an option, I think we usually have more self-explaining names.
This flag will need to be passed multiple times, and should be familiar to any user of this tool, so it seems appropriate to give it a short name.

================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:44
@@ +43,3 @@
+             "     visible outside of the LTO unit\n"
+             "A resolution for each symbol must be specified."),
+    cl::ZeroOrMore);
----------------
mehdi_amini wrote:
> Are there incompatible combination? (I don't think so, but just checking).
I don't think so either.


https://reviews.llvm.org/D20268