[PATCH] D20268: Resolution-based LTO API.

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/LTO/LTO.cpp:318
@@ +317,3 @@
+    RegularLTO.CombinedModule->setTargetTriple(M.getTargetTriple());
+    RegularLTO.CombinedModule->setDataLayout(M.getDataLayout());
+  }
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > The ", we typically have callback hooks into the linker defined where the linker could add its own resolved symbols to the combined module" is not clear to me at all (LTOCodeGenerator will never do that AFAIK for example).
> > (I understand that Gold does something with "commons" at that time, but haven't add time to figure why it is needed) 
> To be more explicit: I don't like this behavior because it creates a relatively strong coupling between the linker and the plugin. The linker changes the module in an unpredictable way that can conflict with assumption that the plugin implementation could make. It makes it harder to follow invariant in the plugin implementation, and makes it easy to break the client of the API (the linker).
> 
True, this is the case now because gold is the only user. I went ahead and committed (temporarily, as it turned out, will recommit after fixing some bot failures) so that I can also get pcc's follow-on patch in that I need. But will also work up a patch to make the creation of the combined module lazy. I'm not sure how to address your other concern about the linker changing the module in unpredictable ways, but the API does document that the linker may add symbols to the combined module via the callback hooks.

================
Comment at: lib/LTO/LTOBackend.cpp:61
@@ +60,3 @@
+      raw_fd_ostream OS(Path, EC, sys::fs::OpenFlags::F_None);
+      if (EC) {
+        // Because -save-temps is a debugging feature, we report the error
----------------
I didn't notice that this change was causing an issue since I had old output files around that caused tests to pass, but this caused a bot failure due to missing output files. The problem is not just that the names are different, but that they become difficult to correlate to the corresponding input source name. E.g. in the gold/X86/thinlto_linkonceresolution.ll test, where we have "%gold ... -o %t3.o %t2.o %t.o", the old .opt.bc temp files were named %t.o.4.opt.bc and %t2.o.4.opt.bc, but with the change I made become %t3.o.1.4.opt.bc and %t3.o.2.4.opt.bc. It isn't at all obvious which output file corresponds to which input file (numbering will depend on the ModuleMap iteration order).

What I did was to revert the change I had made, but add a new bool flag parameter on addSaveTemps (UseInputModulePath) that is set to true by gold and will provoke the old behavior. We can iterate on a better solution. Probably pass in a (temp) directory name and output all the files in a tree rooted there (note that you could have same named module identifiers at different paths, so you can't just disambiguate by appending the basename of the input module). 


https://reviews.llvm.org/D20268