This is an archive of the discontinued LLVM Phabricator instance.

[TLS] use emulated TLS if the target supports only this mode
ClosedPublic

Authored by chh on Feb 6 2018, 6:02 PM.

Details

Summary

Emulated TLS is enabled by llc flag -emulated-tls, which is passed by clang driver.
When llc is called explicitly or from other drivers like LTO, missing -emulated-tls flag would generate wrong TLS code for targets that supports only this mode.
Now use useEmulatedTLS() instead of Options.EmulatedTLS to decide whether emulated TLS code should be generated.
Unit tests are modified to run with and without the -emulated-tls flag.

Diff Detail

Repository
rL LLVM

Event Timeline

chh created this revision.Feb 6 2018, 6:02 PM
chh added a reviewer: chapuni.Feb 6 2018, 6:07 PM

This doesn't seem like the right way to go.

First the easy things:
The llc tool is not intended to be a user-facing tool, so I don't think that justifies it on its own.
This breaks -fno-emulated-tls.

Then, LTO...
For sure, the way that flags get passed down into LTO currently is certainly a quite-irritating set of kludges. However, teaching the clang driver to pass the right flag through to the lto plugin seems entirely possible, and would seem to actually fix the issue more completely, also handling a user-specified -femulated-tls flag on non-android platforms.

chh updated this revision to Diff 133315.Feb 7 2018, 2:42 PM

I don't think -fno-emulated-tls is used anywhere yet.
I should have not allowed that flag.
My original idea was to have only -femulated-tls for
targets like Android.

Anyway I don't want to disable -fno-emulated-tls now.
So the new patch has added ExplicitEmulatedTLS to
indicate that either -emulated-tls or -no-emulated-tls
is passed to code generator.
If ExplicitEmulatedTLS is not set, like current LTO,
we should use the target triple to decide EmulatedTLS.

I don't trust all front-end drivers to set -emulated-tls correctly.
There will be more tools to call llc.
We should have correct target dependent default for code generator.

srhines added inline comments.Feb 13 2018, 9:17 AM
lib/Target/TargetMachine.cpp
195 ↗(On Diff #133315)

Can we move this into a function in Triple as well? I feel like keeping this as a property there makes it more likely to be handled for future users as well.

chh updated this revision to Diff 134078.Feb 13 2018, 10:22 AM
chh marked an inline comment as done.

James, can you comment on whether this meets your expectations here? This works fine with emulated-tls disabled as well.

jyknight accepted this revision.Feb 28 2018, 7:37 AM

It still feels like a hack to workaround LLVM deficiencies. But, good enough.

This revision is now accepted and ready to land.Feb 28 2018, 7:37 AM
This revision was automatically updated to reflect the committed changes.

This broke enabling emulated TLS manually from clang; clang sets Options.EmulatedTLS in llvm::TargetOptions but doesn't yet set Options.ExplicitEmulatedTLS anywhere.

chh added a comment.Mar 1 2018, 2:06 PM

The clang driver will be fixed in https://reviews.llvm.org/D43965