[MLIR] Add `comdat` to globals when translating to LLVM IR on Windows

[mnadeem]

Summary

This patch fixes linker errors about duplicate symbols seen when compiling OpenBLAS using Flang on windows.

Flang string literals are globals with linkonce linkage and identical strings in different files get the same
hashed names. They are handled properly on Linux but cause a linker duplicate symbol error on windows.

Another solution would have been to change the linkage for string literals in Flang to private but I am not
sure if that's appropriate.

I am not very familiar with linkers so please let me know if something seems wrong.

[rnk]

Does the MLIR LLVM dialect not have comdats? That seems like a missing feature, a lack of fidelity that would prevent lossless conversion from LLVM IR to MLIR & back. Clang uses comdat groups for some esoteric features, the D5 destructor alias group, and MSVC RTTI.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

676–677

Use supportsCOMDAT instead of isOSBinFormatCOFF to cover Linux. Linux linkonce_odr functions should also have comdats.

[tschuett]

Shouldn't Flang create the comdats when he is targeting Windows?

[mnadeem]

In D127455#3573920, @rnk wrote:

Does the MLIR LLVM dialect not have comdats? That seems like a missing feature, a lack of fidelity that would prevent lossless conversion from LLVM IR to MLIR & back. Clang uses comdat groups for some esoteric features, the D5 destructor alias group, and MSVC RTTI.

I couldn't find a way to add comdats to MLIR (unless I missed something).

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

676–677

According to https://llvm.org/docs/LangRef.html#linkage-types variables with ODR linkage should have equivalent data, which maps to Comdat::ExactMatch, right? but ELF only supports any and nodeduplicate.

Would it be correct to set Comdat::Any for ODR linkages?

[rnk]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

676–677

Functions are not generally bit for bit identical between TUs, even when they have equivalent definitions, so ExactMatch can't normally be used. Any is the normal behavior, and would match what you currently get on other platforms.

In theory, ExactMatch could be applied to certain kinds of readonly data, but in practice, I have never observed MSVC use this setting.

[mnadeem]

• Update tests
• Check for TargetTriple.supportsCOMDAT() and use the any comdat by default

[mnadeem]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

676–677

Use supportsCOMDAT instead of isOSBinFormatCOFF to cover Linux. Linux linkonce_odr functions should also have comdats.

Updated to use supportsCOMDAT. Btw this code block only deals with mlir::LLVM::GlobalOp which dont include functions.

[rriddle]

In D127455#3574415, @mnadeem wrote:

In D127455#3573920, @rnk wrote:

Does the MLIR LLVM dialect not have comdats? That seems like a missing feature, a lack of fidelity that would prevent lossless conversion from LLVM IR to MLIR & back. Clang uses comdat groups for some esoteric features, the D5 destructor alias group, and MSVC RTTI.

I couldn't find a way to add comdats to MLIR (unless I missed something).

It likely hasn't been necessary at this point, and thus hasn't been added (things in the llvm dialect are generally built in a need based fashion). The best path forward would likely be to build out support for comdats in the LLVM dialect and leave the decision of when to build those up to flang/whatever frontend.

[mnadeem]

Removing from reviewer's ready to review list for now.

[luporl]

I have noticed that duplicate symbol linker errors don't occur anymore, when using current flang on Windows.
Now comdat appears after string constants, as in the snippet below:

@_QQcl.48656C6C6F20 = linkonce constant [6 x i8] c"Hello ", comdat

My guess is that D153768 fixed it.

Is this patch still needed?

[mnadeem]

Redundant after https://reviews.llvm.org/D150796 and https://reviews.llvm.org/D153768

flang/test/Fir/global.fir

 // RUN: tco %s | FileCheck %s
 // RUN: %flang_fc1 -emit-llvm  %s -o - | FileCheck %s
 
+// CHECK: $w_i86 = comdat any
+// CHECK: $str1 = comdat any
+
 // CHECK: @g_i0 = global i32 0
 fir.global @g_i0 : i32 {
   %1 = arith.constant 0 : i32
   fir.has_value %1 : i32
 }
 
 // CHECK: @g_i2 = global i32 2
 fir.global @g_i2 : i32 {
   %1 = arith.constant 2 : i32
   fir.has_value %1 : i32
 }
 
 // CHECK: @g_ci5 = constant i32 5
 fir.global @g_ci5 constant : i32 {
   %c = arith.constant 5 : i32
   fir.has_value %c : i32
 }
 
 // CHECK: @i_i515 = internal global i32 515
 fir.global internal @i_i515 (515:i32) : i32
 
 // CHECK: @C_i511 = common global i32 0
 fir.global common @C_i511 (0:i32) : i32
 
-// CHECK: @w_i86 = weak global i32 86
+// CHECK: @w_i86 = weak global i32 86, comdat
 fir.global weak @w_i86 (86:i32) : i32
 
-// CHECK: @str1 = global [6 x i8] c"Hello!"
-fir.global @str1 : !fir.char<1,6> {
+// CHECK: @str1 = linkonce global [6 x i8] c"Hello!", comdat
+fir.global linkonce @str1 : !fir.char<1,6> {
   %1 = fir.string_lit "Hello!"(6) : !fir.char<1,6>
   fir.has_value %1 : !fir.char<1,6>
 }
 
 // CHECK: @_QEmask = internal global [32 x i32] [i32 1, i32 1
 fir.global internal @_QEmask : !fir.array<32xi32> {
   %c0_i32 = arith.constant 1 : i32
   %0 = fir.undefined !fir.array<32xi32>

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

auto *var = new llvm::GlobalVariable(
         addrSpace);
 
     if (op.getUnnamedAddr().hasValue())
       var->setUnnamedAddr(convertUnnamedAddrToLLVM(*op.getUnnamedAddr()));
 
     if (op.getSection().hasValue())
       var->setSection(*op.getSection());
 
+    auto TargetTriple = llvm::Triple(llvmModule->getTargetTriple());
+    if (var->isWeakForLinker() && !var->hasCommonLinkage() &&
+        !var->isDeclarationForLinker() && TargetTriple.supportsCOMDAT())
+      var->setComdat(llvmModule->getOrInsertComdat(var->getName()));
+
     addRuntimePreemptionSpecifier(op.getDsoLocal(), var);
 
     Optional<uint64_t> alignment = op.getAlignment();
     if (alignment.hasValue())
       var->setAlignment(llvm::MaybeAlign(alignment.getValue()));
 
     globalsMapping.try_emplace(op, var);
   }

mlir/test/Target/LLVMIR/llvmir.mlir

 // RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
 
+// CHECK: $linkonce = comdat any
+// CHECK: $weak = comdat any
+// CHECK: $linkonce_odr = comdat any
+// CHECK: $weak_odr = comdat any
+
 // CHECK: @global_aligned32 = private global i64 42, align 32
 "llvm.mlir.global"() ({}) {sym_name = "global_aligned32", global_type = i64, value = 42 : i64, linkage = #llvm.linkage<private>, alignment = 32} : () -> ()
 
 // CHECK: @global_aligned64 = private global i64 42, align 64
 llvm.mlir.global private @global_aligned64(42 : i64) {alignment = 64 : i64} : i64
 
 // CHECK: @global_aligned64_native = private global i64 42, align 64
 llvm.mlir.global private @global_aligned64_native(42 : i64) { alignment = 64 } : i64

 //
 
 // CHECK: @private = private global i32 42
 llvm.mlir.global private @private(42 : i32) : i32
 // CHECK: @internal = internal global i32 42
 llvm.mlir.global internal @internal(42 : i32) : i32
 // CHECK: @available_externally = available_externally global i32 42
 llvm.mlir.global available_externally @available_externally(42 : i32) : i32
-// CHECK: @linkonce = linkonce global i32 42
+// CHECK: @linkonce = linkonce global i32 42, comdat
 llvm.mlir.global linkonce @linkonce(42 : i32) : i32
-// CHECK: @weak = weak global i32 42
+// CHECK: @weak = weak global i32 42, comdat
 llvm.mlir.global weak @weak(42 : i32) : i32
 // CHECK: @common = common global i32 0
 llvm.mlir.global common @common(0 : i32) : i32
 // CHECK: @appending = appending global [3 x i32] [i32 1, i32 2, i32 3]
 llvm.mlir.global appending @appending(dense<[1,2,3]> : tensor<3xi32>) : !llvm.array<3xi32>
 // CHECK: @extern_weak = extern_weak global i32
 llvm.mlir.global extern_weak @extern_weak() : i32
-// CHECK: @linkonce_odr = linkonce_odr global i32 42
+// CHECK: @linkonce_odr = linkonce_odr global i32 42, comdat
 llvm.mlir.global linkonce_odr @linkonce_odr(42 : i32) : i32
-// CHECK: @weak_odr = weak_odr global i32 42
+// CHECK: @weak_odr = weak_odr global i32 42, comdat
 llvm.mlir.global weak_odr @weak_odr(42 : i32) : i32
 // CHECK: @external = external global i32
 llvm.mlir.global external @external() : i32
 
 //
 // UnnamedAddr attribute.
 //
 

 
 llvm.mlir.global internal constant @taker_of_address() : !llvm.ptr<func<void ()>> {
   %0 = llvm.mlir.addressof @address_taken : !llvm.ptr<func<void ()>>
   llvm.return %0 : !llvm.ptr<func<void ()>>
 }
 
 // -----
 
-// CHECK: @forward_use_of_address = linkonce global ptr @address_declared_after_use
+// CHECK: $forward_use_of_address = comdat any
+// CHECK: @forward_use_of_address = linkonce global ptr @address_declared_after_use, comdat
 llvm.mlir.global linkonce @forward_use_of_address() : !llvm.ptr<f32> {
   %0 = llvm.mlir.addressof @address_declared_after_use : !llvm.ptr<f32>
   llvm.return %0 : !llvm.ptr<f32>
 }
 
 llvm.mlir.global linkonce @address_declared_after_use() : f32
 
 // -----
 
-// CHECK: @take_self_address = linkonce global { i32, ptr } {{.*}} ptr @take_self_address
+// CHECK: $take_self_address = comdat any
+// CHECK: @take_self_address = linkonce global { i32, ptr } {{.*}} ptr @take_self_address }, comdat
 llvm.mlir.global linkonce @take_self_address() : !llvm.struct<(i32, !llvm.ptr<i32>)> {
   %z32 = llvm.mlir.constant(0 : i32) : i32
   %0 = llvm.mlir.undef : !llvm.struct<(i32, !llvm.ptr<i32>)>
   %1 = llvm.mlir.addressof @take_self_address : !llvm.ptr<!llvm.struct<(i32, !llvm.ptr<i32>)>>
   %2 = llvm.getelementptr %1[%z32, 0] : (!llvm.ptr<!llvm.struct<(i32, !llvm.ptr<i32>)>>, i32) -> !llvm.ptr<i32>
   %3 = llvm.insertvalue %z32, %0[0 : i32] : !llvm.struct<(i32, !llvm.ptr<i32>)>
   %4 = llvm.insertvalue %2, %3[1 : i32] : !llvm.struct<(i32, !llvm.ptr<i32>)>
   llvm.return %4 : !llvm.struct<(i32, !llvm.ptr<i32>)>