This is an archive of the discontinued LLVM Phabricator instance.

[llvm][codegen] Disallow default Emulated TLS for RISCV
AbandonedPublic

Authored by paulkirth on Feb 8 2023, 6:02 PM.

Details

Summary

Based on https://github.com/llvm/llvm-project/issues/59500, and further
discussion on https://reviews.llvm.org/D102527, it seems that we will
not support Emulated TLS for RISCV in LLVM. In these cases, we should
avoid setting defaults for a feature we will not support.

This also avoids some trouble when using LTO on targets that would
normally default to emulated TLS, since it can be challenging to
propagate frontend flags like -fno-emulated-tls correctly.

Diff Detail

Event Timeline

paulkirth created this revision.Feb 8 2023, 6:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 6:02 PM
paulkirth requested review of this revision.Feb 8 2023, 6:02 PM

Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?
Or are you essentially asserting, by making this contribution, that we should add additional opt-outs for other architectures?

The test overall looks reasonable but it could be cleaned up a little bit (e.g. the attributes).

Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?

I'm not completely sure, but AFAIK this list accurate. What I do know is that we don't support this feature for triples w/ riscv64 in them.

Or are you essentially asserting, by making this contribution, that we should add additional opt-outs for other architectures?

I think we should follow suite whenever the architecture(or our policy w.r.t. the architecture) has a similar limitation. I'm not aware of any other architectures with that limitation, though. In this case, we won't support emulated TLS, and having this default set incorrectly leads to some hard to diagnose issues w/ undefined variables showing up under LTO. Ideally, we'd propagate the fno-emulated-tls flag through to the linker better, but I think flags not making it through under LTO is a bigger/more general issue and would require non-trivial work + an RFC. But, in this case the default is still wrong for 64-bit RISC-V.

The test overall looks reasonable but it could be cleaned up a little bit (e.g. the attributes).

Thanks for taking a look. I'll try to minimize the test a bit more.

jrtc27 added inline comments.Feb 9 2023, 8:55 AM
llvm/include/llvm/TargetParser/Triple.h
960–961

Hoisting this out to supportsEmulatedTLS() would seem useful, and would be reusable by Clang to diagnose -femulated-tls on such targets

paulkirth updated this revision to Diff 496167.Feb 9 2023, 10:10 AM

Add supportsEmulatedTLS API, and try to minimize test.

paulkirth marked an inline comment as done.Feb 9 2023, 10:10 AM
paulkirth updated this revision to Diff 496174.Feb 9 2023, 10:22 AM

Revise comments in test file

asb added inline comments.Feb 9 2023, 10:25 AM
llvm/include/llvm/TargetParser/Triple.h
955

Shouldn't this return false for 32-bit RISC-V as well?

paulkirth added inline comments.Feb 9 2023, 10:38 AM
llvm/include/llvm/TargetParser/Triple.h
955

Maybe? I was only aware of the 64-bit limitation. If the situation is the same, I'll update this to include !isRISCV32() as well.

paulkirth updated this revision to Diff 496179.Feb 9 2023, 10:52 AM

Disable emulated TLS for all RISCV. Update comments in test that only mention RISCV64

paulkirth marked an inline comment as done.Feb 9 2023, 10:52 AM

Thanks for doing this. LGTM, defer to code owners for approval.

MaskRay added inline comments.Feb 9 2023, 1:26 PM
lld/test/ELF/lto/riscv-no-emutls.ll
1 ↗(On Diff #496179)
paulkirth updated this revision to Diff 496252.Feb 9 2023, 3:05 PM

Fix layering violation in test

paulkirth marked an inline comment as done.Feb 9 2023, 3:08 PM
paulkirth added inline comments.
lld/test/ELF/lto/riscv-no-emutls.ll
1 ↗(On Diff #496179)

I think the new test under CodeGen/LTO/RISCV should be better situated, but please let me know if you see another problem w/ the test structure.

vit9696 requested changes to this revision.Feb 10 2023, 12:00 AM
vit9696 added a subscriber: vit9696.

This change looks inadequate to me. As I explicitly stated in the mentioned issue, we need emutls support for RISC-V. For the reasons stated there I believe we should enable emutls support for RISC-V as GCC does, and this differential should be abandoned.

Just to clarify, emutls support is literally as simple as adding these three lines:

diff -rupN a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp	2022-06-22 19:46:24
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp	2022-12-14 11:48:02
@@ -3718,6 +3718,9 @@ SDValue RISCVTargetLowering::lowerGlobalTLSAddress(SDV
 
 SDValue RISCVTargetLowering::lowerGlobalTLSAddress(SDValue Op,
                                                    SelectionDAG &DAG) const {
+  GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(Op);
+  if (DAG.getTarget().useEmulatedTLS())
+    return LowerToTLSEmulatedModel(GA, DAG);
   SDLoc DL(Op);
   EVT Ty = Op.getValueType();
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);
This revision now requires changes to proceed.Feb 10 2023, 12:00 AM

This change looks inadequate to me. As I explicitly stated in the mentioned issue, we need emutls support for RISC-V. For the reasons stated there I believe we should enable emutls support for RISC-V as GCC does, and this differential should be abandoned.

Just to clarify, emutls support is literally as simple as adding these three lines:

diff -rupN a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp	2022-06-22 19:46:24
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp	2022-12-14 11:48:02
@@ -3718,6 +3718,9 @@ SDValue RISCVTargetLowering::lowerGlobalTLSAddress(SDV
 
 SDValue RISCVTargetLowering::lowerGlobalTLSAddress(SDValue Op,
                                                    SelectionDAG &DAG) const {
+  GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(Op);
+  if (DAG.getTarget().useEmulatedTLS())
+    return LowerToTLSEmulatedModel(GA, DAG);
   SDLoc DL(Op);
   EVT Ty = Op.getValueType();
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);

The code owners of the RISCV backend seem to disagree as pointed out in that issue and the linked differential on phabricator.

As it stands we miscompile code that sets these options. Until the RISC-V backend correctly handles TLS emulation we should prevent such issues. If the change you suggest lands, this can be reverted.

Until then we should not ship compilers that work incorrectly and that silently miscompile user code. I think maintaining that invariant is much more important than parity or conformity with other compilers.

@paulkirth, I agree that shipping a working compiler is more important than parity with the other compilers, but in this particular case we simply face a simple to fix bug caused by component owner tyranny :) We should simply fix this bug and move forward.

@paulkirth, I agree that shipping a working compiler is more important than parity with the other compilers, but in this particular case we simply face a simple to fix bug caused by component owner tyranny :) We should simply fix this bug and move forward.

Declaring LLVM maintainers tyrants is neither helpful nor appropriate.

@jrtc27, I agree and am very sorry for that having been taken personally. To make things more constructive I submitted a new differential (D143708), where I also summarised the current state of the art with the emulated TLS on RISC-V.

asb added a comment.EditedFeb 10 2023, 7:55 AM

To put another perspective on issue #59500 - I'd say there was some initial discussion that trailed off over the christmas/new year period. It's a seemingly simple change, but people who are experts in this area raised concern about whether it's necessary/worthwhile. Your patch in D143708 helpfully summarises the key arguments and also points to information that was new to me (OpenBSD carry a patch for this downstream and thus it has presumably been tested and solves their problem). I'm sorry if the experience of discussing this missing feature is frustrating, but I hope you can also see the other side. I don't think there's "tyranny" (we'd need a universally agreed viewpoint for that!), just a group of contributors trying their best.

I'll continue the discussion about enabling emulated TLS in D143708. @vit9696 perhaps you could remove the "requires changes to proceed" marker, unless you have specific quality of implementation concerns with this patch? It might be we decide to abandon it in favour of enabling emulated TLS, but even if it's just the case that it takes a little longer to agree on that, this patch is still better than the status quo where it sounds like we're generating incorrect code and might be reasonable stop-gap.

paulkirth updated this revision to Diff 496507.Feb 10 2023, 9:03 AM

Fix typo in test.

this patch is still better than the status quo where it sounds like we're generating incorrect code and might be reasonable stop-gap.

+1. The status quo isn't desirable.

brad added a subscriber: brad.Feb 10 2023, 11:51 AM

Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?

For OpenBSD, yes.

vit9696 resigned from this revision.Feb 10 2023, 12:10 PM

@vit9696 perhaps you could remove the "requires changes to proceed" marker, unless you have specific quality of implementation concerns with this patch? It might be we decide to abandon it in favour of enabling emulated TLS, but even if it's just the case that it takes a little longer to agree on that, this patch is still better than the status quo where it sounds like we're generating incorrect code and might be reasonable stop-gap.

Ok.

To all: please do not rush with this patch, as long as the RISC-V implementation is merged, we will have very few reasons to merge it even after changes.

enh added a subscriber: enh.Feb 13 2023, 8:38 AM

Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?

For OpenBSD, yes.

For Android too.

ELF TLS is a relatively new addition to Android. riscv64 is the only target architecture added more recently than that, and there aren't yet enough devices in the field with ELF TLS for us to have looked at changing clang's default for Android to be based on API level yet. (it's probably around the 60% mark at this point, though, so it's getting to the point where we should do this.)

paulkirth abandoned this revision.Feb 16 2023, 8:25 AM

Based on discussion at the LLVM RISC-V community call, we will be adding emulated TLS support, so this patch can be abandoned.