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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
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:
> tejohnson wrote:
> > pcc wrote:
> > > joker.eph wrote:
> > > > pcc wrote:
> > > > > 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.
> > > > I agree with this being a single interface for the linker specific part. The LTO and ThinLTO part don't have to be expose in this header. This can be private part of the implementation.
> > > > Same for whatever needs to be refactor out of the ThinLTOCodeGenerator: these are not to be exposed in this header.
> > > > 
> > > > 
> > > > > I think in the long term we will probably want the distributed backends using this class as well
> > > > 
> > > > I don't see why would you use this class? Here we deal with linker specific stuff like symbol resolution and so on. The backend is another layer and should deal with the importing/linkage decisions and IR as an input, and produce a single object as an output.
> > > > 
> > > > 
> > > > The backend is another layer and should deal with the importing/linkage decisions and IR as an input, and produce a single object as an output.
> > > 
> > > Yes, that's roughly what the ThinPartition (or whatever we call it, maybe ThinBackend would be a better name) would be doing. That would be the layer shared between the distributed and in-process backends. Now that I think about it, there's probably no need to use the LTO class itself in the distributed backends.
> > > Now that I think about it, there's probably no need to use the LTO class itself in the distributed backends.
> > 
> > Yes and I think it shouldn't be in the same library, but rather in Transforms/IPO. I also think there might be some issues adding a dependence on LTO from elsewhere in the pipeline, since it depends on many of the other libraries already. 
> > Same for whatever needs to be refactor out of the ThinLTOCodeGenerator: these are not to be exposed in this header.
> 
> Initially it needs to be, until this interface is in place. Will send the patch hopefully later today or early tomorrow after I clean it up, will make the refactored routines standalone in a new LTO.h/LTO.cpp for now.
> Yes and I think it shouldn't be in the same library, but rather in Transforms/IPO.

I'm not sure that IPO would be the right place for ThinBackend (it's not really a compiler pass), but that's where the function importer lives so probably a good enough place for now.


http://reviews.llvm.org/D20268