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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D23681#520280, @mehdi_amini wrote:

> In https://reviews.llvm.org/D23681#519900, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D23681#519878, @mehdi_amini wrote:
> >
> > > In https://reviews.llvm.org/D23681#519861, @tejohnson wrote:
> > >
> > > > Looks like we crossed wires a bit - see my https://reviews.llvm.org/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).


https://reviews.llvm.org/D23681