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

[Teresa Johnson via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

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.

> 

> 

> > 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.


https://reviews.llvm.org/D23681