[RISCV] Support emulated TLS

[vit9696]

Summary

As discussed earlier in the GitHub issue, currently LLVM generates invalid code when emulated TLS is used. There were attempts to resolve this previously (D102527), but they were not merged because the component owners raised concerns about emulated TLS efficiency.

The current state of the art is that:

• OpenBSD team, which raised the initial issue, simply has patches downstream.
• Our team, which raised the GH issue, has patches downstream as well. We also do not use malloc or any dynamic allocations with emulated TLS, so the concerns raised in the original issue does not apply to us.
• GCC compatibility is broken, because GCC supports emulated TLS.
• RISC-V is the only architecture in LLVM that does not support emulated TLS, and work is being done to at least warn the users about it (D143619).

With all these in mind I believe it is important to address the consumers' needs especially given that there is little to no maintenance downsides.

[vit9696]

Added a test case.

[asb]

I left a few comments in D143619 about the history of this discussion. Thanks for the clear summary of scenarios where this is needed and an example where someone is already carrying a downstream patch.

From the discussion so far:

• In #59500, @kito-cheng raised the point that in theory emulated TLS shouldn't be necessary
• In #59500, @jrtc27 stated "Up until now it’s been a deliberate decision to not support emulated TLS, it’s terrible for performance and adds toolchain complexity."
• In D102527, @MaskRay expressed the hope that emutls could be removed from LLVM and that OpenBSD could stop relying on it, and @brad also hoped the OpenBSD developers could do this

So, what's changed / what new arguments have been raised since then?

• Partly nothing - it seems there's no movement on OpenBSD removing emutls support, and it seems OpenBSD are now carrying a patch downstream to enable it. On the plus side, that at least implies additional confirmation that this patch meets their requirements.
• @vit9696 has another use case that needs this "We make private use of generic emutls abi on one of our targets for portability and easier certification reasons."
• It was pointed out that rather than just not supporting emulated TLS, we're accepting the flag and generating incorrect code. So _something_ needs to be done.
• @vit9696 points out -femulated-tls would give parity with GCC. That's normally a compelling argument, though if LLVM were on the verge of removing emulated TLS for all targets it would be less compelling. As there's been no movement on this, perhaps it's not likely to happen any time soon?

In light of all of the above, what are your views @jrtc27, @MaskRay, @kito-cheng? I'll also note we have the regular RISC-V LLVM contributor call next Thu @ 4pm GMT. It may be we can resolve this discussion on this thread, but if not, that call might be a good venue for further discussion.

[MaskRay]

I want to know more about "We make private use of generic emutls abi on one of our targets for portability and easier certification reasons."

Adding the support is easy, but that just adds more on top of a legacy interface and invites maintenance burden.
It's true that nobody is actively removing emutls, but that's different from being willing to accept more dependency on the legacy interface. The more legacy users we have, the more difficult we are able to remove when we pull the trigger to remove the support.
I do occasionally check what Android workarounds can be removed and have removed quite a few in the past.

In addition, being different from the rest of the world has the disadvantage that you are on your own. Things work perfectly on other systems may have mysterious failures on your system. You may complain about changes that accidentally break your system. To avoid this, we can just disallow further uses of a legacy interface.

If some OpenBSD developers want to study how ELF TLS works. Debugging musl rtld is a good start.
For the general topic, my https://maskray.me/blog/2021-02-14-all-about-thread-local-storage has covered many concepts.

[vit9696]

@MaskRay, we have a family of private in-house RTOSes for safety-critical application with static resource allocation (memory, threads, processes, etc). Our supported architectures include AArch64, ARM, MIPS, PowerPC, RISC-V, X86 and we have TLS in all of them. For easier certification, unified codebase, and compatibility reasons with various kinds of gcc and clang we opted with emulated TLS API. However, instead of using the implementation from builtins we went with our own, which looks roughly like this:

void* __emutls_get_address(emutls_control* control) {
    return &data[control->object.index + get_thread_id() * tls_thread_size]
}

While we might add native TLS API for some architectures, our current implementation is fast enough for our needs, and the code modification process is rather costly. Also, as far as I know, most compilers do not even support TLS e.g. on MIPS without trapping, so for some processors it is not even possible to get rid of the emulated TLS without rewriting the compiler. Usually the latter is also not an option at scale.

[asb]

Just a reminder this is on the agenda for the RISC-V LLVM sync-up call tomorrow. We can of course set up some other discussion if a call would be a good way to move this forward and tomorrow's usual RISC-V call time isn't convenient. But even in the absence of that, if anyone else wants to add thoughts to this thread that's very helpful.

@enh reported in D143619 that this is somewhat relevant to Android, as Clang's default for emulated/non-emulated TLS is changed based on API level yet.

[asb]

The consensus from the RISC-V LLVM sync-up call was that it makes sense to go ahead with this on the basis their are users, the infrastructure in LLVM already exists to use it etc, and it's a tiny patch to enable etc.

In which case, assuming no-one who wasn't able to make it to the call wanted to raise strong objections, we can hopefully focus on code review of this patch and move it forwards.

[MaskRay]

In D143708#4132222, @asb wrote:

The consensus from the RISC-V LLVM sync-up call was that it makes sense to go ahead with this on the basis their are users, the infrastructure in LLVM already exists to use it etc, and it's a tiny patch to enable etc.

In which case, assuming no-one who wasn't able to make it to the call wanted to raise strong objections, we can hopefully focus on code review of this patch and move it forwards.

I have a mild objection but I am fine if my opinion is overthrown. I think OpenBSD developers should take on the work to migrate to ELF TLS, not using this definitely.

-femulated-tls can be used for the current *-linux-android* and *-openbsd* target triples, and OpenBSD RISC-V, but not others. The test should be changed to use openbsd, not linux-gnu.

If we ever encounter features using TLS that are difficult to work with emulated TLS, acceptance of this patch doesn't mean that we want to accept additional complexity.

[jrtc27]

In D143708#4133064, @MaskRay wrote:

In D143708#4132222, @asb wrote:

The consensus from the RISC-V LLVM sync-up call was that it makes sense to go ahead with this on the basis their are users, the infrastructure in LLVM already exists to use it etc, and it's a tiny patch to enable etc.

In which case, assuming no-one who wasn't able to make it to the call wanted to raise strong objections, we can hopefully focus on code review of this patch and move it forwards.

I have a mild objection but I am fine if my opinion is overthrown. I think OpenBSD developers should take on the work to migrate to ELF TLS, not using this definitely.

-femulated-tls can be used for the current *-linux-android* and *-openbsd* target triples, and OpenBSD RISC-V, but not others. The test should be changed to use openbsd, not linux-gnu.

Or just bare metal.

[vit9696]

This is actually the triple we use, despite our OS not having any relevance to Linux, mainly since LLVM has no better triple for us. I basically added the triple we personally need to work with.

[MaskRay]

llvm/test/CodeGen/RISCV/emutls_generic.ll

43 ↗

(On Diff #496377)

32-bit and 64-bit can share many CHECK prefixes. Use --check-prefixes=CHECK,CHECK-64 and --check-prefixes=CHECK,CHECK-32.

; RISCV_64: __emutls_get_address isn't useful to know which reference it is. Test the whole instruction like call __emutls_get_address

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4572

Hoist N instead

4574

Newline after

llvm/test/CodeGen/RISCV/emutls_generic.ll

2 ↗

(On Diff #496377)

_ in CHECK prefixes is unusual, but normally we do RV32/64

3 ↗

(On Diff #496377)

Why do we need optimisation tests?

8 ↗

(On Diff #496377)

32 before 64

20 ↗

(On Diff #496377)

Why's that in an emutls test? And isn't that implied by our TLS tests not having anything to do with emutls in the generated code?

22 ↗

(On Diff #496377)

Who cares about order?

28 ↗

(On Diff #496377)

These need codegen check lines, for which you should be using update_llc_test_checks.py

43 ↗

(On Diff #496377)

This isn't readable, at least indent things properly, though I forget if update_llc_test_checks.py can do globals in assembly... I think not?

44 ↗

(On Diff #496377)

Distinct lack of -NEXT everywhere

54 ↗

(On Diff #496377)

Why does the lack of a suffix matter? Seems a bit odd to pick this specifically.

63 ↗

(On Diff #496377)

What on earth is .rdata?

[brad]

In D143708#4133064, @MaskRay wrote:

I have a mild objection but I am fine if my opinion is overthrown. I think OpenBSD developers should take on the work to migrate to ELF TLS, not using this definitely.

-femulated-tls can be used for the current *-linux-android* and *-openbsd* target triples, and OpenBSD RISC-V, but not others. The test should be changed to use openbsd, not linux-gnu.

If we ever encounter features using TLS that are difficult to work with emulated TLS, acceptance of this patch doesn't mean that we want to accept additional complexity.

It's easier said that done. I have been asking about this going back like 15 years and it's basically silence from the relevant developers.

[paulkirth]

Just a drive-by observation, but the test in this patch is basically a verbatim copy of CodeGen/AArch64/emutls_generic.ll. If we're going to mandate changes to this test it seems to me that other tests for this feature should also be changed.

[jrtc27]

Whilst I would encourage wherever this got copied from be cleaned up in a similar manner, I don't think:

1. this patch for one backend should be blocked on cleaning up a test in another backend
2. a lower-quality test for another backend should be justification for committing a lower-quality test to this backend

Having had to poke at the AArch64 backend before a lot of the tests are really quite poor in quality; we're trying to stop that happening with the RISCV backend.

[vit9696]

Sorry for the delay, got a bit sick here. I mostly acked the requested changes and left comments on the ones I believe should not be applied. Will try to prepare the V2 by the end of the week.

llvm/test/CodeGen/RISCV/emutls_generic.ll

3 ↗

(On Diff #496377)

To make sure there are no oddities with the optimisation passes against emutls.

20 ↗

(On Diff #496377)

It is not implied. Negative checking is mandatory for any good test, in my opinion.

22 ↗

(On Diff #496377)

The original test does, but I kind of expect this too.

63 ↗

(On Diff #496377)

.rdata is used in PE/COFF instead of .rodata. As long as Microsoft adds RISC-V support this is how this code may be generated.

[jrtc27]

Didn't notice before, but why emutls_generic.ll rather than emutls.ll? The suffix doesn't add any value as far as I can see. It's as generic as any other RISC-V CodeGen test.

llvm/test/CodeGen/RISCV/emutls_generic.ll

1 ↗

(On Diff #496377)

These should be bare-metal triples.

3 ↗

(On Diff #496377)

Well:

1. llc defaults to -O2
2. you could say that about anything, emutls isn't special
3. if it works with optimisations it'll almost certainly work without

Axe this.

20 ↗

(On Diff #496377)

It *is* implied. The non-emutls tests do not contain calls to anything emutls. Negative checking is *not* mandatory, in fact it's *bad* for a CodeGen test because it's a hand-written CHECK line that can't be automated. Plus CHECK-NOT can be quite hard to use correctly.

Axe this.

63 ↗

(On Diff #496377)

But you're generating code for ELF. ELF does not have .rdata.

[vit9696]

llvm/test/CodeGen/RISCV/emutls_generic.ll

54 ↗

(On Diff #496377)

This is because for non-emutls symbols are generated in .data.symbolname sections.

[jrtc27]

llvm/test/CodeGen/RISCV/emutls_generic.ll

54 ↗

(On Diff #496377)

No they're not, real TLS symbols are in .tdata/.tbss

[vit9696]

llvm/test/CodeGen/RISCV/emutls_generic.ll

54 ↗

(On Diff #496377)

This is most likely target-specific then. This is what we observed with -ffunction-sections -fdata-sections on I *think* MIPS in the past. I am fine to drop this part though.

[vit9696]

Added V2. Should have applied everything. I did not split the common part, because after the changes it became rather small.

[vit9696]

Apparently my build was out of date and update_llc_test_checks.py generated incompatible results. Fixed in V3.

[hiraditya]

LGTM, i'll let the code owners make the final call. Thanks for working on this.

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4576

Should this assert apply? Given it's true of ELF TLS I'd expect it of emulated TLS too?

llvm/test/CodeGen/RISCV/emutls.ll

2–5 ↗

(On Diff #499300)

Triple should come first and < %s should come last

109 ↗

(On Diff #499300)

Please indent all these to look like how the assembly is formatted, same as is done for matching code

117 ↗

(On Diff #499300)

Why all these NOTs?

[vit9696]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4576

Is this offset within the TLS space? If so, I am not sure it cannot be 0 for emutls.

I do not have enough knowledge of the codebase to make this decision, and I am reluctant to make a blind change. Can apply if you insist & explain why it should be safe.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4576

I think it should always be 0 because RISCVTargetLowering::isOffsetFoldingLegal returns false.

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4576

In fact TargetLowering::LowerToTLSEmulatedModel also asserts this. So yes, safe to hoist it too.

[vit9696]

Addressed the review comments.

[arichardson]

llvm/test/CodeGen/RISCV/emutls.ll

108 ↗

(On Diff #502096)

You probably want to add ; UTC_ARGS: --disable here so that re-running the script doesn't remove those manual check lines.

[vit9696]

llvm/test/CodeGen/RISCV/emutls.ll

108 ↗

(On Diff #502096)

Thanks, added)

[asb]

I think all the review comments have been addressed and this LGTM (thanks!) - though let's wait for confirmation from @jrtc27 before committing.

[brad]

@jrtc27 Ping?

[jrtc27]

llvm/test/CodeGen/RISCV/emutls.ll

7 ↗

(On Diff #502115)

I still don’t see what the point of this comment is

10 ↗

(On Diff #502115)

This isn’t external?

17 ↗

(On Diff #502115)

Use nounwind to avoid these

[vit9696]

llvm/test/CodeGen/RISCV/emutls.ll

10 ↗

(On Diff #502115)

Yes, due to internal_y with the same name.

[jrtc27]

llvm/test/CodeGen/RISCV/emutls.ll

10 ↗

(On Diff #502115)

Huh? You can change y to something else... and you probably should, having two y-based variables is unnecessarily confusing when you could just use z.

I forget if initialised globals without a specified linkage are in fact technically regarded as external, but we don't let you write external there in the IR so it seems a bit confusing to put it in the variable name.

[vit9696]

llvm/test/CodeGen/RISCV/emutls.ll

10 ↗

(On Diff #502115)

This tests whether it works correctly when internal and external symbols are present.

Either way, I am fine with this change if you insist. Since it is the last change requested and I do not have commit access, could you please handle it on your own and push to master? Thanks!

[jrtc27]

llvm/test/CodeGen/RISCV/emutls.ll

10 ↗

(On Diff #502115)

I would prefer the test as committed go through proper pre-commit review

[vit9696]

llvm/test/CodeGen/RISCV/emutls.ll

10 ↗

(On Diff #502115)

Actually wait.

@external_y = thread_local global i8 7, align 2

external thread_local global i8 does not allow size to be specified here. The test is correct as is.

[jrtc27]

llvm/test/CodeGen/RISCV/emutls.ll

10 ↗

(On Diff #502115)

That is not my point. My point is that the name says external_y, which does not match the fact that the RHS doesn't have (and as you correctly state, does not support) external.

[vit9696]

Ok, I think I finally understood what you meant now. I kindly request you to be more explicit next time, also providing a more complete review from the start would help to reduce the amount of iterations and time needed for the change to land.

Updated.

[MaskRay]

No objection. LG once @jrtc27 is happy.

[asb]

In D143708#4212196, @MaskRay wrote:

No objection. LG once @jrtc27 is happy.

@jrtc27 - ping on this.

[jrtc27]

This is a lot cleaner now, thanks

[asb]

Brilliant, thanks everyone. @vit9696 am I right that you don't currently have LLVM commit access - would you like me to go ahead and commit this for you?

[vit9696]

In D143708#4231142, @asb wrote:

Brilliant, thanks everyone. @vit9696 am I right that you don't currently have LLVM commit access - would you like me to go ahead and commit this for you?

Sure, tyvm :-)

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

CLI.setDebugLoc(DL)
                     DAG.getExternalSymbol("__tls_get_addr", Ty),
                     std::move(Args));
 
   return LowerCallTo(CLI).first;
 }
 
 SDValue RISCVTargetLowering::lowerGlobalTLSAddress(SDValue Op,
                                                    SelectionDAG &DAG) const {
+  GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(Op);
+  if (DAG.getTarget().useEmulatedTLS())
+    return LowerToTLSEmulatedModel(GA, DAG);
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);
   assert(N->getOffset() == 0 && "unexpected offset in global node");
 
   TLSModel::Model Model = getTargetMachine().getTLSModel(N->getGlobal());
 
   if (DAG.getMachineFunction().getFunction().getCallingConv() ==
       CallingConv::GHC)
     report_fatal_error("In GHC calling convention TLS is not supported");
 
   SDValue Addr;