[LTO] Add the ability to test -thinlto-emit-imports-files through llvm-lto2

[mehdi_amini]

Summary

Start bringing llvm-lto2 to a level where we can test the LTO API
a bit deeper.

[tejohnson]

Looks like we crossed wires a bit - see my D23680 patch. We have chosen slightly different options names (I chose thinlto-index-only to be more consistent with the gold-plugin). I added a similar llvm-lto2 based test to an existing llvm-lto based test.

Do you have a preference on which version of the llvm-lto support and test to take? Since you have also added the Threads option maybe we should use yours (but I think I like thinlto-index-only better for the option), and I can rebase on top. WDYT?

[mehdi_amini]

In D23681#519861, @tejohnson wrote:

Looks like we crossed wires a bit - see my D23680 patch. We have chosen slightly different options names (I chose thinlto-index-only to be more consistent with the gold-plugin).

Interestingly I copy/pasted the option name from the gold-plugin!!

I added a similar llvm-lto2 based test to an existing llvm-lto based test.

I plan to kill the support for this option in llvm-lto in a subsequent patch, in which I'll convert the existing llvm-lto test to llvm-lto2, that's the main reason I submitted a new test here (I extracted this change from the other patch, and figured I'd submit a test for this patch and wrote one), but I'm fine just using the other test as well instead.

Do you have a preference on which version of the llvm-lto support and test to take? Since you have also added the Threads option maybe we should use yours (but I think I like thinlto-index-only better for the option), and I can rebase on top. WDYT?

I don't have a strong preference, but I'd add the llvm-lto2 support separately.

[mehdi_amini]

I don't have a strong preference, but I'd add the llvm-lto2 support separately.

(I mean separately from your other patch)

[tejohnson]

In D23681#519878, @mehdi_amini wrote:

In D23681#519861, @tejohnson wrote:

Looks like we crossed wires a bit - see my D23680 patch. We have chosen slightly different options names (I chose thinlto-index-only to be more consistent with the gold-plugin).

Interestingly I copy/pasted the option name from the gold-plugin!!

That particular option only has an effect under thinlto-index-only, which guards the creation of the WriteIndexesThinBackend and subsequent early exit from the link. For my patch I decided in llvm-lto2 to just enable both under the single parent option, similar to how you are enabling both (writing the individual indexes and the imports files as well) with one option. I think it is better to have that one option be the thinlto-index-only since it is the main option for this path on the plugin side.

I added a similar llvm-lto2 based test to an existing llvm-lto based test.

I plan to kill the support for this option in llvm-lto in a subsequent patch, in which I'll convert the existing llvm-lto test to llvm-lto2, that's the main reason I submitted a new test here (I extracted this change from the other patch, and figured I'd submit a test for this patch and wrote one), but I'm fine just using the other test as well instead.

Would it be easier then to use the same test for both llvm-lto and llvm-lto2 then? Will make killing off one of them easier (just modifying the one test rather than removing files).

Do you have a preference on which version of the llvm-lto support and test to take? Since you have also added the Threads option maybe we should use yours (but I think I like thinlto-index-only better for the option), and I can rebase on top. WDYT?

I don't have a strong preference, but I'd add the llvm-lto2 support separately.

Ok, why don't we add it separately with this patch (but with the option name changed). I'm ok either way on the test.

LGTM with the option name change.

[mehdi_amini]

In D23681#519900, @tejohnson wrote:

In D23681#519878, @mehdi_amini wrote:

In D23681#519861, @tejohnson wrote:

Looks like we crossed wires a bit - see my D23680 patch. We have chosen slightly different options names (I chose thinlto-index-only to be more consistent with the gold-plugin).

Interestingly I copy/pasted the option name from the gold-plugin!!

That particular option only has an effect under thinlto-index-only, which guards the creation of the WriteIndexesThinBackend and subsequent early exit from the link. For my patch I decided in llvm-lto2 to just enable both under the single parent option, similar to how you are enabling both (writing the individual indexes and the imports files as well) with one option. I think it is better to have that one option be the thinlto-index-only since it is the main option for this path on the plugin side.

In llvm-lto there was three options: thinlink (write only the index), distributedindexes (write the individual indexes for distributed backends), and emitimports (write the list of imports for each module).

So how do you see the mapping? One option for all of these together? The name "thinlto-index-only" makes me think about the first one only.

[tejohnson]

In D23681#520280, @mehdi_amini wrote:

In D23681#519900, @tejohnson wrote:

In D23681#519878, @mehdi_amini wrote:

In D23681#519861, @tejohnson wrote:

Looks like we crossed wires a bit - see my D23680 patch. We have chosen slightly different options names (I chose thinlto-index-only to be more consistent with the gold-plugin).

Interestingly I copy/pasted the option name from the gold-plugin!!

That particular option only has an effect under thinlto-index-only, which guards the creation of the WriteIndexesThinBackend and subsequent early exit from the link. For my patch I decided in llvm-lto2 to just enable both under the single parent option, similar to how you are enabling both (writing the individual indexes and the imports files as well) with one option. I think it is better to have that one option be the thinlto-index-only since it is the main option for this path on the plugin side.

In llvm-lto there was three options: thinlink (write only the index), distributedindexes (write the individual indexes for distributed backends), and emitimports (write the list of imports for each module).

So how do you see the mapping? One option for all of these together? The name "thinlto-index-only" makes me think about the first one only.

For gold, I implemented it such that thinlto-index-only controlled the execution behavior and generation of individual indexes. The thinlto-emit-imports-files only has an effect under thinlto-index-only, because presumably you only need those if you are also creating individual index files and exiting before the backends.

The new LTO API has the same model - i.e. EmitImportsFiles parameter only has an effect if we are creating a WriteIndexes backend.

For llvm-lto, I kept the same model for the other ThinLTO features tested there and set it up so you could test each one individually (e.g. emit just the imports files, and not the individual indexes). But for llvm-lto2, since it is using the new API, emitting the imports files is only an option if already emitting the individual indexes.

The gold-plugin doesn't really have an equivalent to "thinlink" (just write combined index and quit).

I agree though that thinlto-index-only doesn't really express what we are doing here either. Perhaps "thinlto-distributed-indexes" is a bit more general? Not sure it is worth having a separate flag to enable/disable the imports file emission, think it is reasonable to enable both under one option as you have done here (and as I did in my patch from today).

[mehdi_amini]

Reuse the llvm-lto test, and change the option name to -thinlto-distributed-indexes

[tejohnson]

Suggest changing name of variable EmitImportsFiles to ThinLTODistributedIndexes to match new option.

[mehdi_amini]

Rename to ThinLTODistributedIndexes

test/ThinLTO/X86/emit_imports.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/emit_imports.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc
 ; RUN: llvm-lto -thinlto-action=emitimports -thinlto-index %t.index.bc %t1.bc %t2.bc
 
 ; The imports file for this module contains the bitcode file for
 ; Inputs/emit_imports.ll
 ; RUN: cat %t1.bc.imports | count 1
 ; RUN: cat %t1.bc.imports | FileCheck %s --check-prefix=IMPORTS1
 ; IMPORTS1: emit_imports.ll.tmp2.bc
 
 ; The imports file for Input/emit_imports.ll is empty as it does not import anything.
 ; RUN: cat %t2.bc.imports | count 0
 
+; RUN: rm -f %t*.thinlto.bc %t*.bc.imports
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o \
+; RUN:     -thinlto-distributed-indexes \
+; RUN:     -r=%t1.bc,g, \
+; RUN:     -r=%t1.bc,f,px \
+; RUN:     -r=%t2.bc,g,px
+
+; RUN: cat %t1.bc.imports | count 1
+; RUN: cat %t1.bc.imports | FileCheck %s --check-prefix=IMPORTS1
+
+; The imports file for Input/emit_imports.ll is empty as it does not import anything.
+; RUN: cat %t2.bc.imports | count 0
+
 declare void @g(...)
 
 define void @f() {
 entry:
   call void (...) @g()
   ret void
 }

tools/llvm-lto2/llvm-lto2.cpp

static cl::list<std::string> InputFilenames(cl::Positional, cl::OneOrMore,
                                             cl::desc("<input bitcode files>"));
 
 static cl::opt<std::string> OutputFilename("o", cl::Required,
                                            cl::desc("Output filename"),
                                            cl::value_desc("filename"));
 
 static cl::opt<bool> SaveTemps("save-temps", cl::desc("Save temporary files"));
 
+static cl::opt<bool> EmitImportFiles("thinlto-distributed-indexes",
+                                     cl::init(false),
+                                     cl::desc("Write out individual index and "
+                                              "import files for the "
+                                              "distributed backend case"));
+
+static cl::opt<int> Threads("-thinlto-threads",
+                            cl::init(thread::hardware_concurrency()));
+
 static cl::list<std::string> SymbolResolutions(
     "r",
     cl::desc("Specify a symbol resolution: filename,symbolname,resolution\n"
              "where \"resolution\" is a sequence (which may be empty) of the\n"
              "following characters:\n"
              " p - prevailing: the linker has chosen this definition of the\n"
              "     symbol\n"
              " l - local: the definition of this symbol is unpreemptable at\n"
int main(int argc, char **argv) {
   Conf.DiagHandler = [](const DiagnosticInfo &) {
     exit(1);
   };
 
   if (SaveTemps)
     check(Conf.addSaveTemps(OutputFilename + "."),
           "Config::addSaveTemps failed");
 
-  LTO Lto(std::move(Conf));
+  ThinBackend Backend;
+  if (EmitImportFiles)
+    Backend = createWriteIndexesThinBackend("", "", true, "");
+  else
+    Backend = createInProcessThinBackend(Threads);
+  LTO Lto(std::move(Conf), std::move(Backend));
 
   bool HasErrors = false;
   for (std::string F : InputFilenames) {
     std::unique_ptr<MemoryBuffer> MB = check(MemoryBuffer::getFile(F), F);
     std::unique_ptr<InputFile> Input =
         check(InputFile::create(MB->getMemBufferRef()), F);
 
     std::vector<SymbolResolution> Res;