[PATCH] D20268: [wip] Resolution-based LTO API.

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:25
@@ +24,3 @@
+  /// be in this linkage unit.
+  unsigned DefinitionInLinkageUnit : 1;
+
----------------
Basically, does it mean we can add visibility hidden in the IR?
And if not Prevailing, it means we can internalize?

================
Comment at: include/llvm/LTO/LTO.h:48
@@ +47,3 @@
+                            symbol_iterator(Obj->symbol_end()));
+  }
+
----------------
Not sure why is it part of this class.

================
Comment at: include/llvm/LTO/LTO.h:88
@@ +87,3 @@
+           ArrayRef<SymbolResolution> Res,
+           DiagnosticHandlerFunction DiagHandler);
+
----------------
I'd wrap IRObjectFile + Resolution in a separate class... Oh, LTOModule is back.

================
Comment at: include/llvm/LTO/LTO.h:95
@@ +94,3 @@
+  /// The callee must set up an output stream to write the native object to, then
+  /// call CG passing the output stream as a parameter.
+  ///
----------------
Why not using a `raw_pwrite_stream ` return value instead of passing another callback?
What is the "Task" param?



================
Comment at: include/llvm/LTO/LTO.h:111
@@ +110,3 @@
+  /// function returns false, any further processing for that task is aborted.
+  typedef std::function<bool(size_t Task, Module &)> ModuleHookFn;
+
----------------
Define task.

================
Comment at: include/llvm/LTO/LTO.h:117
@@ +116,3 @@
+  /// This module hook is called between optimization and code generation.
+  ModuleHookFn PostOptModuleHook;
+
----------------
I'm not convinced by a fixed set of hook like that. Look at the save-temps in ThinLTOCodeGenerator.

================
Comment at: include/llvm/LTO/LTO.h:122
@@ +121,3 @@
+  /// unique task identifier between 0 and getNumTasks()-1.
+  size_t getNumTasks() const;
+
----------------
I don't see why we need this if we have appropriate callbacks. This may not be known before the end of the process.
The callbacks you're defining above can be used to add file/stream on the fly.

================
Comment at: include/llvm/LTO/LTO.h:126
@@ +125,3 @@
+  /// functions to add native object files to the link.
+  bool codegen(AddStreamFn AddStream, AddFileFn AddFile);
+
----------------
incorrect naming if it is supposed to perform more than just the codegen.

================
Comment at: include/llvm/LTO/LTO.h:137
@@ +136,3 @@
+  std::string TheTriple;
+  const Target *TheTarget;
+  struct GlobalResolution {
----------------
All the member above are not making any sense here for ThinLTO.

================
Comment at: include/llvm/LTO/LTO.h:142
@@ +141,3 @@
+  };
+  StringMap<GlobalResolution> GlobalResolutions;
+};
----------------
Describe.


http://reviews.llvm.org/D20268