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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: include/llvm/LTO/LTO.h:113
@@ +112,3 @@
+  /// getMaxTasks()-1, which is supplied to the callback via the Task parameter.
+  /// There are two exceptions to the general rule that a task represents an
+  /// entire pipeline:
----------------
tejohnson wrote:
> Is the general rule then for ThinLTO? Otherwise, I think regular LTO with no parallel code gen has 1 task, right?
Yes to both. I will update this to be more explicit about each type of backend.

================
Comment at: include/llvm/LTO/LTO.h:138
@@ +137,3 @@
+  typedef std::function<LTONativeObjectStream(size_t Task)> AddStreamFn;
+
+  /// This type defines a file callback. The file callback passed to run() is
----------------
tejohnson wrote:
> If we have mixed ThinLTO and LTO mode, would you have one instance of this class for the ThinLTO portion and one for the regular LTO portion, or is it trying to manage both? If the former, does it make sense to add derived classes to handle each, so that the class doesn't contain methods and members that only are used in the other type?
> 
> Also, I have done the refactoring of the ThinLTOCodeGeneration handling for ODR resolution and internalization to operate via the Index. For now, I have them still in the same ThinLTOCodeGeneration.cpp file, but need to move them out to somewhere that can be shared by the various linkers. This new file seems like a good place for those that are applicable to the ThinLTO thin link step (i.e. consume resolution info and update the index). If we can have a derived class here for ThinLTO (thin link) I would put those here. The others (consume the index and do the actual ODR linkage changes and internalization) happen in the ThinLTO backends, which seems better suited to Transforms/IPO, especially since we will have the distributed backends coming in via clang.
I want to have it manage both, but the layering would be done inside the class. I was thinking about something like 

```
class LTO {
 ...
  class RegularPartition {
    // implementation of regular LTO
  };
  class ThinPartition {
    // implementation of ThinLTO backend
  };
  // implementation of rest of ThinLTO pipeline

  RegularPartition Regular;
  std::vector<ThinPartition> Thin; // one ThinPartition per input file
};
```

> This new file seems like a good place for those that are applicable to the ThinLTO thin link step (i.e. consume resolution info and update the index).

Yes, it could probably be done as a static member function of LTO as that would be implementing the thin link phase. Eventually I think we would probably want to make that a private member function once all linkers are using this class.

> The others (consume the index and do the actual ODR linkage changes and internalization) happen in the ThinLTO backends, which seems better suited to Transforms/IPO, especially since we will have the distributed backends coming in via clang.

I think in the long term we will probably want the distributed backends using this class as well (via a slightly different interface, or at least they should be able to share the implementation of ThinPartition), so maybe that can be made a free function in this header for now.


http://reviews.llvm.org/D20268