Page MenuHomePhabricator

add -fthinlto-index= option to clang-cl
ClosedPublic

Authored by inglorion on Jul 9 2019, 5:25 PM.

Details

Summary

This adds a -fthinlto-index= option to clang-cl, which allows it to
be used to drive ThinLTO backend passes. This allows clang-cl to be
used for distributed ThinLTO.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Jul 9 2019, 5:25 PM

This is similar to r254927 for the clang driver, and reuses the tests from that commit for the backend part. On the frontend, it differs in that clang-cl does not support -x (per the comment in clang/lib/Driver/Driver.cpp line 2068: "// No driver mode exposes -x and /TC or /TP; we don't support mixing them.") Instead of requiring -x ir, the behavior added to clang-cl here is to expect that what would be detected as a native object file based on file extension is really a bitcode file. If it isn't, the backend will cause us to say "error: Invalid bitcode signature", which I think is adequate as a diagnostic.

pcc added a comment.Jul 9 2019, 5:35 PM

Do we really need to support clang-cl syntax here? Can't the user of -fthinlto-index= use the regular clang driver?

rnk added a comment.Jul 9 2019, 6:13 PM
In D64458#1577435, @pcc wrote:

Do we really need to support clang-cl syntax here? Can't the user of -fthinlto-index= use the regular clang driver?

I think it depends on how many compiler flags we have to pass to backend compilation actions. I don't have a good sense of how many of those actually matter. Some flags affect the object file and are not encoded in bitcode, like -ffunction-sections. It's potentially onerous for a build system to have to work out for itself that cl -O2 implicitly enables -Gy, which is the MSVC spelling of -ffunction-sections, so therefore we need this other set of backend action flags. It seems nicer to for the build system to just take the first stage command line and re-run it with a new input and this new flag.

If we want to make the behavior consistent, I would say we should port this new -x ir behavior over to clang so it's the same as clang-cl.

Do we really need to support clang-cl syntax here? Can't the user of -fthinlto-index= use the regular clang driver?

We could, of course, require using the clang driver for this feature. But I would argue that clang-cl exists to make it easy for projects that use MSVC-style command lines to switch to Clang and get access to the benefits it provides, including features for which there is no equivalent MSVC flag. So I think this is reasonable to add.

LGTM, but please wait for @pcc to make sure he is satisfied.
I think it would be good to port the file type deduction to clang (possibly as a follow on), to make these consistent.

inglorion updated this revision to Diff 209954.Jul 15 2019, 2:09 PM

Simplified after rebasing on top of r366127.

rnk accepted this revision.Jul 15 2019, 3:08 PM

lgtm too, since now this is just whitelisting a clang flag for clang-cl.

This revision is now accepted and ready to land.Jul 15 2019, 3:08 PM
pcc added a comment.Jul 15 2019, 3:14 PM

I think it's a little unfortunate that we're continuing to go down the road of letting users pass more flags to the ThinLTO backend action, but I won't stand in the way here.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 3:50 PM