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

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

I like Lang approach to decouple a bit more the configuration.


================
Comment at: include/llvm/CodeGen/LTOBackend.h:59
@@ +58,3 @@
+  /// pipeline. For example, a linker may use this function to implement
+  /// -save-temps, or to add its own resolved symbols to the module. If this
+  /// function returns false, any further processing for that task is aborted.
----------------
"to add its own resolved symbols": so the linker will modify the module? (I notice the Module is not const in the hook prototype)
What is the use-case?

================
Comment at: include/llvm/CodeGen/LTOBackend.h:62
@@ +61,3 @@
+  ///
+  /// Module hooks must be thread safe.
+  typedef std::function<bool(size_t Task, Module &)> ModuleHookFn;
----------------
What level of "thread-safety" are we talking about? Is it just about the linker internal structure? Can the linker safely assume that only one thread will call one of these callbacks for a given Module? For a given LLVMContext?

================
Comment at: include/llvm/CodeGen/LTOBackend.h:67
@@ +66,3 @@
+  /// (ThinLTO) the module, before modifying it.
+  ModuleHookFn PreOptModuleHook;
+
----------------
What is the Task "ID" at this point? This would be a task that will not codegen in the case of a parallel backend?

================
Comment at: include/llvm/CodeGen/LTOBackend.h:94
@@ +93,3 @@
+  typedef std::function<bool(ModuleSummaryIndex &Index)> CombinedIndexHookFn;
+  CombinedIndexHookFn CombinedIndexHook;
+};
----------------
If the intent is just -save-temps, I'd rather have the Index passed as a const ref.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:107
@@ +106,3 @@
+/// completion, or false if a hook aborted the backend by returning false.
+bool backend(Config &C, StringRef TheTriple, const Target *TheTarget,
+             AddStreamFn AddStream, unsigned ParallelCodeGenParallelismLevel,
----------------
Why the Triple string and Target here?

================
Comment at: include/llvm/CodeGen/LTOBackend.h:120
@@ +119,3 @@
+/// If GV's linkage is linkonce, change it to weak so that it won't be DCE'd.
+void upgradeLinkage(GlobalValue *GV);
+
----------------
This is a strange API at this level. 
And the naming is very generic for what the description says it does.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:136
@@ +135,3 @@
+  DiagnosticHandlerFunction DiagHandler;
+};
+
----------------
Why this `LTOLLVMContext` instead of a factory method in the Config class?

================
Comment at: include/llvm/LTO/LTO.h:90
@@ +89,3 @@
+  comdat_alias = 1,
+  unable_to_find_target,
+};
----------------
I think some doc may be nice for the enum values (I have no idea what is `comdat_alias` right now).

================
Comment at: include/llvm/LTO/LTO.h:93
@@ +92,3 @@
+
+const std::error_category &lto_category();
+
----------------
doc (public API in public header)

================
Comment at: include/llvm/LTO/LTO.h:292
@@ +291,3 @@
+  LTO(Config Conf, ThinBackend Backend = nullptr,
+      unsigned ParallelCodeGenParallelismLevel = 1);
+
----------------
Not clear why `ParallelCodeGenParallelismLevel` is not part of `Config`.
Also it does not apply to ThinLTO, which is not clear.

================
Comment at: include/llvm/LTO/LTO.h:323
@@ +322,3 @@
+  ModuleSummaryIndex CombinedIndex;
+  StringMap<MemoryBufferRef> ModuleMap;
+
----------------
There is too much state here. 
I could see getting rid of almost *all* of these.
Until you call `run()` on this class, I don't expect that we need any but the `Config` and `ModuleMap`.

================
Comment at: include/llvm/LTO/LTO.h:385
@@ +384,3 @@
+
+  std::vector<std::unique_ptr<object::IRObjectFile>> ThinObjs;
+
----------------
Why do we need this here?


http://reviews.llvm.org/D20268