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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

A few comments on the API.


================
Comment at: include/llvm/LTO/LTO.h:51
@@ +50,3 @@
+public:
+  LTO(LLVMContext &Ctx);
+
----------------
I don't expect a single context here, describe what you have in mind.

================
Comment at: include/llvm/LTO/LTO.h:56
@@ +55,3 @@
+  unsigned ParallelCodeGenParallelismLevel = 1;
+  unsigned ThinLTOParallelismLevel = 1;
+
----------------
Lack of encapsulation: I'd rather create a nested `Options` class/struct (or a global `LTOOptions` one) and pass it to `run()`

================
Comment at: include/llvm/LTO/LTO.h:141
@@ +140,3 @@
+
+  /// A combined index hook is called after all module indices have been
+  /// combined. It can be used to implement -save-temps for the combined index.
----------------
Not sure what "indices" mean in this context.

================
Comment at: include/llvm/LTO/LTO.h:155
@@ +154,3 @@
+  /// it could copy the index, input file and import files to a remote machine
+  /// and run the backend task there.
+  ///
----------------
I'm not sure I'm convinced this is the right level to implement that, i.e. delegating to the linker the orchestration.

================
Comment at: include/llvm/LTO/LTO.h:157
@@ +156,3 @@
+  ///
+  /// The caller must set up and return an output stream to write the index to.
+  typedef std::function<std::unique_ptr<raw_ostream>(
----------------
Do you mean callee? I don't understand how this API works.

================
Comment at: include/llvm/LTO/LTO.h:159
@@ +158,3 @@
+  typedef std::function<std::unique_ptr<raw_ostream>(
+      size_t Task, StringRef InputFile, ArrayRef<StringRef> ImportFiles)>
+      ThinBackendHookFn;
----------------
`StringRef InputFile` seems to assume that  there is an actual path to a file, while it could be a buffer in memory.

================
Comment at: include/llvm/LTO/LTO.h:167
@@ +166,3 @@
+
+  LLVMContext &getContext() const { return Ctx; }
+
----------------
I don't expect to see this here, what is the use case?

================
Comment at: include/llvm/LTO/LTO.h:206
@@ +205,3 @@
+  // IRMover and all possible cross-partition references have been seen.
+  StringMap<GlobalResolution> GlobalResolutions;
+
----------------
I'm not understanding clearly this right now, I'll have to come back to this later (as well for all the private APIs below).



http://reviews.llvm.org/D20268