This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mnadeem on Jun 9 2022, 4:19 PM.

Details

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.

Diff Detail

Event Timeline

mnadeem created this revision.Jun 9 2022, 4:19 PM
Herald added a project: Restricted Project. · View Herald Transcript
mnadeem requested review of this revision.Jun 9 2022, 4:19 PM
rnk added a comment.Jun 10 2022, 9:48 AM

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.

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

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 added inline comments.Jun 13 2022, 3:33 PM
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.

kiranchandramohan resigned from this revision.Jun 14 2022, 2:55 AM
clementval resigned from this revision.Jun 14 2022, 3:45 AM
mnadeem updated this revision to Diff 436981.Jun 14 2022, 5:30 PM
mnadeem retitled this revision from [MLIR][COFF] Add `comdat` to globals when translating to LLVM IR on Windows to [MLIR] Add `comdat` to globals when translating to LLVM IR on Windows.
mnadeem removed reviewers: kiranchandramohan, clementval.
  • Update tests
  • Check for TargetTriple.supportsCOMDAT() and use the any comdat by default
mnadeem marked an inline comment as done.Jun 14 2022, 5:33 PM
mnadeem added inline comments.
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.

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 planned changes to this revision.Feb 27 2023, 11:52 AM

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

luporl added a subscriber: luporl.Sep 4 2023, 12:58 PM

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?