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

[paulkirth]

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.

[luismarques]

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

[paulkirth]

In D143619#4115147, @luismarques wrote:

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]

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]

Add supportsEmulatedTLS API, and try to minimize test.

[paulkirth]

Revise comments in test file

[asb]

llvm/include/llvm/TargetParser/Triple.h

955

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

[paulkirth]

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]

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

[hiraditya]

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

[hiraditya]

Also cc: @craig.topper

[MaskRay]

lld/test/ELF/lto/riscv-no-emutls.ll

1 ↗

(On Diff #496179)

The test is added to the wrong layering https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer

[paulkirth]

Fix layering violation in test

[paulkirth]

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.

[paulkirth]

git clang-format

[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);

[paulkirth]

In D143619#4117333, @vit9696 wrote:

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.

[vit9696]

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

[jrtc27]

In D143619#4117399, @vit9696 wrote:

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

[vit9696]

@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]

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]

Fix typo in test.

[hiraditya]

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]

In D143619#4115147, @luismarques wrote:

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

For OpenBSD, yes.

[vit9696]

@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]

In D143619#4119248, @brad wrote:

In D143619#4115147, @luismarques wrote:

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]

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

llvm/include/llvm/TargetParser/Triple.h

public:
   }
 
   /// Tests whether the target supports comdat
   bool supportsCOMDAT() const {
     return !(isOSBinFormatMachO() || isOSBinFormatXCOFF() ||
              isOSBinFormatDXContainer());
   }
 
+  /// Tests whether the target supports emulated tls
+  bool supportsEmulatedTLS() const {
+    return !isRISCV();
+  }
+
   /// Tests whether the target uses emulated TLS as default.
   bool hasDefaultEmulatedTLS() const {
-    return isAndroid() || isOSOpenBSD() || isWindowsCygwinEnvironment();
+    return supportsEmulatedTLS() &&
+           (isAndroid() || isOSOpenBSD() || isWindowsCygwinEnvironment());
   }
 
   /// Tests whether the target uses -data-sections as default.
   bool hasDefaultDataSections() const {
     return isOSBinFormatXCOFF() || isWasm();
   }
 
   /// Tests if the environment supports dllimport/export annotations.

llvm/test/LTO/RISCV/riscv-no-emutls.ll

+; Ensure targets that normally enable emulated TLS by default, don't run the
+; LowerEmuTLSPass under LTO when targeting RISCV. Under LTO the backend options
+; for emulated TLS won't be propagated unless passed explicitly as part of the
+; linker invocation. Since LLVM doesn't support emulated TLS on RISCV, we want
+; to be sure related transforms don't take place.
+
+; RUN: opt < %s -o %t.bc --mtriple=riscv64-unknown-linux-android10000
+
+; RUN: llvm-lto2 run %t.bc -o %t.out.o  \
+; RUN:     -r=%t.bc,baz,pxl \
+; RUN:     -debug-pass=Structure 2>&1 | FileCheck %s --allow-empty
+; RUN: llvm-readelf -s %t.out.o.1 | FileCheck %s --check-prefix=NOUNDEF
+
+; Ensure the backend pass we want to avoid doesn't run: LowerEmuTLSPass
+; so we check for its output under pass structure
+; CHECK-NOT: Add __emutls_
+
+; Also check that we don't end up creating internal thread_locals that are
+; undefined in the final link. This used to happen because the LowerEmuTLSPass
+; would be added when targeting platforms with default emulated TLS.
+; NOUNDEF    : TLS     LOCAL  DEFAULT  {[0-9]+} Mem
+; NOUNDEF-NOT: TLS     GLOBAL DEFAULT  UND Mem
+
+target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
+
+%struct.Memory = type { i32, i32 }
+
+@Mem = internal thread_local global ptr null, align 8
+
+define void @baz(i32 noundef signext %n) {
+entry:
+  %loc = inttoptr i64 66 to ptr
+  %threadlocal = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @Mem)
+  store ptr %loc, ptr %threadlocal, align 8
+
+  ret void
+}
+
+declare nonnull ptr @llvm.threadlocal.address.p0(ptr nonnull)