[ConstantMerge] Don't merge thread_local constants with non-thread_local constants

[Amanieu]

Summary

Fixes PR49932

[lebedev.ri]

Don't we just need to ensure that we preserve thread_local attr?

[Amanieu]

It's a bit more tricky than that:

• I'm not sure how we should handle the case where we are trying to merge 2 thread_local constants with different TLS models.
• In general we are better off *not* merging global constants with TLS constants since calculating a TLS address can be expensive.

[Amanieu]

Merge thread_locals only if they use the same TLS mode

[Amanieu]

Ping.

[Amanieu]

Ping

[mehdi_amini]

Reading the bug description, it seems that if the TLS constant as an unnamed address we should be able to merge it into a non-TLS constant?

[Amanieu]

I'm not even sure what it would *mean* to have an unnamed TLS constant: I would expect an earlier pass to convert it to a normal constant rather than having to it in ConstantMerge.

Also, AFAIK neither clang nor rustc will ever generate unnamed TLS constants.

[efriedma]

Can we just completely forbid merging thread_local constants (in isUnmergeableGlobal)? I don't really want to think about the implications of merging.

[mehdi_amini]

In D100322#2723566, @Amanieu wrote:

I'm not even sure what it would *mean* to have an unnamed TLS constant: I would expect an earlier pass to convert it to a normal constant rather than having to it in ConstantMerge.

Also, AFAIK neither clang nor rustc will ever generate unnamed TLS constants.

Makes sense, thanks!
Seems like Eli's suggestion would be best then?

[rjmccall]

In D100322#2723566, @Amanieu wrote:

I'm not even sure what it would *mean* to have an unnamed TLS constant: I would expect an earlier pass to convert it to a normal constant rather than having to it in ConstantMerge.

Also, AFAIK neither clang nor rustc will ever generate unnamed TLS constants.

No declared objects in C are ever unnamed_addr, but it's completely reasonable to infer it from use patterns, so it's not like this is an invalid combination. But I agree that it's pointless to merge thread-local constants in constant-merging, because yeah, in any situation where it's permitted we should always be promoting the variable to a global instead (and then maybe that enables constant merging on the promoted TLS).

[Amanieu]

Don't merge thread-local constants

[mehdi_amini]

LGTM

llvm/lib/Transforms/IPO/ConstantMerge.cpp

 }
 
 static bool
 isUnmergeableGlobal(GlobalVariable *GV,
                     const SmallPtrSetImpl<const GlobalValue *> &UsedGlobals) {
   // Only process constants with initializers in the default address space.
   return !GV->isConstant() || !GV->hasDefinitiveInitializer() ||
          GV->getType()->getAddressSpace() != 0 || GV->hasSection() ||
+         // Don't touch thread-local variables.
+         GV->isThreadLocal() ||
          // Don't touch values marked with attribute(used).
          UsedGlobals.count(GV);
 }
 
 enum class CanMerge { No, Yes };
 static CanMerge makeMergeable(GlobalVariable *Old, GlobalVariable *New) {
   if (!Old->hasGlobalUnnamedAddr() && !New->hasGlobalUnnamedAddr())
     return CanMerge::No;

llvm/test/Transforms/ConstantMerge/dont-merge.ll

define void @test4(i32** %P1, i32** %P2, i32** %P3, i32** %P4, i32** %P5, i32** %P6, i32** %P7, i32** %P8) {
         store i32* @T4B1, i32** %P3
         store i32* @T4B2, i32** %P4
         store i32* @T4C1, i32** %P5
         store i32* @T4C2, i32** %P6
         store i32* @T4D1, i32** %P7
         store i32* @T4D2, i32** %P8
         ret void
 }
+
+; CHECK: @T5tls
+; CHECK: @T5ua
+
+@T5tls = private thread_local constant i32 555
+@T5ua = private unnamed_addr constant i32 555
+
+define void @test5(i32** %P1, i32** %P2) {
+        store i32* @T5tls, i32** %P1
+        store i32* @T5ua, i32** %P2
+        ret void
+}