This is an archive of the discontinued LLVM Phabricator instance.

enable -flto=thin in clang-cl
ClosedPublic

Authored by inglorion on Feb 21 2017, 7:58 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Feb 21 2017, 7:58 PM

Can you please add a ThinLTO test to lld before adding driver support?

In D30239#683116, @pcc wrote:

Can you please add a ThinLTO test to lld before adding driver support?

Is clang-cl using lld as default? How is the switch done? Ideally we should have a nice error message from the driver if -flto is used without lld.

@mehdi_amini:

Is clang-cl using lld as default? How is the switch done? Ideally we should have a nice error message from the driver if -flto is used without lld.

I believe we use link.exe by default. You can use lld by passing -fuse-ld=lld to the compiler.

I can add an error message when -flto is used without -fuse-ld=lld at least for the case when linking is actually performed. Of course, it's possible to invoke clang-cl without it doing any linking. If you're only compiling, it's perfectly valid to use -flto without -fuse-ld=lld.

hans edited edge metadata.Feb 22 2017, 11:36 AM

@mehdi_amini:

Is clang-cl using lld as default? How is the switch done? Ideally we should have a nice error message from the driver if -flto is used without lld.

I believe we use link.exe by default. You can use lld by passing -fuse-ld=lld to the compiler.

I can add an error message when -flto is used without -fuse-ld=lld at least for the case when linking is actually performed. Of course, it's possible to invoke clang-cl without it doing any linking. If you're only compiling, it's perfectly valid to use -flto without -fuse-ld=lld.

That sounds like a good idea.

test/Driver/cl-lto.c
2 ↗(On Diff #89315)

This test and the other file look like they're doing the same as test/Driver/(thin)lto.c.

I don't think we need to test these separately for clang-cl; it should be enough with a simple test in test/Driver/cl-options.c to check that they're exposed and that -### looks right for them.

inglorion updated this revision to Diff 89577.Feb 23 2017, 3:42 PM
inglorion retitled this revision from enable -flto=thin, -flto-jobs=, and -fthinlto-index= in clang-cl to enable -flto=thin in clang-cl.

Implemented @hans's suggestion of moving the tests into cl-options.c.

Also restricted the implementation to only implement -flto=. -flto-index= doesn't seem useful without further changes (it just complains that it requires -x ir, which is not supported by clang-cl). -flto-jobs would require a fair bit of extra work compared to what is here now, and I'm not sure it's worth it. The number of jobs lld-link uses can be controlled with /link:/opt:lldltojobs=N, anyway. We can implement support for this later if we want it - for now I really just want -flto=thin.

hans accepted this revision.Feb 23 2017, 3:57 PM

lgtm with a comment

test/Driver/cl-options.c
525 ↗(On Diff #89577)

This needs -- before %s, otherwise if %s expands to e.g. /Users/foo it will be interpreted as the /U option :-)

Same thing below.

This revision is now accepted and ready to land.Feb 23 2017, 3:57 PM
inglorion added inline comments.Feb 23 2017, 4:01 PM
test/Driver/cl-options.c
525 ↗(On Diff #89577)

Good catch. That's what I get for not copy-pasting it. ;-)

inglorion updated this revision to Diff 89583.Feb 23 2017, 4:14 PM

added missing --

inglorion updated this revision to Diff 89590.Feb 23 2017, 4:52 PM

fail early with a friendlier message when using -flto without -fuse-ld=lld

hans added inline comments.Feb 23 2017, 5:00 PM
test/Driver/cl-options.c
532 ↗(On Diff #89590)

This is nit-picky, but "invalid argument" doesn't sound great to me here; -flto isn't invalid, it's the lack of -fuse-ld=lld that's the problem.

Maybe just adding a new "err_drv_flto_without_lld" that says somethihng like "'-flto' without '-fuse-ld=lld' is not supported" would be better?

inglorion updated this revision to Diff 89598.Feb 23 2017, 6:04 PM

changed error message

This revision was automatically updated to reflect the committed changes.
rnk edited edge metadata.Mar 3 2017, 1:42 PM

Do you guys think that maybe -flto should imply -fuse-ld=lld, or is that too much magic?

In D30239#691972, @rnk wrote:

Do you guys think that maybe -flto should imply -fuse-ld=lld, or is that too much magic?

I don't know enough about windows, but I would think it may be surprising for a user that -flto changes the linker behind his back?

I'm biased but as a user I rather have to opt-in instead of the compiler doing stuff without me knowing about it.