This is an archive of the discontinued LLVM Phabricator instance.

[mlir] mlir-opt: Fix linking after 7c4e8c6a273f2 .
ClosedPublic

Authored by dtzWill on Aug 23 2023, 5:51 AM.

Details

Summary

Without this, undefined refernces to the LLVMIR translations:

ld: mlir-opt.cpp:(.text.startup.main+0x49): undefined reference to `mlir::registerAMXDialectTranslation(mlir::DialectRegistry&)'
ld: mlir-opt.cpp:(.text.startup.main+0x51): undefined reference to `mlir::registerArmSMEDialectTranslation(mlir::DialectRegistry&)'
ld: mlir-opt.cpp:(.text.startup.main+0x59): undefined reference to `mlir::registerArmSVEDialectTranslation(mlir::DialectRegistry&)'
ld: mlir-opt.cpp:(.text.startup.main+0x81): undefined reference to `mlir::registerOpenACCDialectTranslation(mlir::DialectRegistry&)'
ld: mlir-opt.cpp:(.text.startup.main+0x89): undefined reference to `mlir::registerOpenMPDialectTranslation(mlir::DialectRegistry&)'
ld: mlir-opt.cpp:(.text.startup.main+0x99): undefined reference to `mlir::registerX86VectorDialectTranslation(mlir::DialectRegistry&)'

Diff Detail

Event Timeline

dtzWill created this revision.Aug 23 2023, 5:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 5:51 AM
dtzWill requested review of this revision.Aug 23 2023, 5:51 AM

Is a bot broken? What kind of config do you use?

fmorac added a subscriber: fmorac.Aug 24 2023, 5:27 AM

To avoid increasing binary size the following patch should be enough to fix the issue:

diff --git a/mlir/tools/mlir-opt/mlir-opt.cpp b/mlir/tools/mlir-opt/mlir-opt.cpp
index b47634476c62..5cd1b40ea6c9 100644
--- a/mlir/tools/mlir-opt/mlir-opt.cpp
+++ b/mlir/tools/mlir-opt/mlir-opt.cpp
@@ -276,7 +276,14 @@ int main(int argc, char **argv) {
   DialectRegistry registry;
   registerAllDialects(registry);
   registerAllExtensions(registry);
-  registerAllToLLVMIRTranslations(registry);
+
+  // TODO: Remove these when a more structured extension registration mechanism
+  // is added.
+  registerBuiltinDialectTranslation(registry);
+  registerGPUDialectTranslation(registry);
+  registerLLVMDialectTranslation(registry);
+  registerNVVMDialectTranslation(registry);
+  registerROCDLDialectTranslation(registry);
 
 #ifdef MLIR_INCLUDE_TESTS
   ::test::registerTestDialect(registry);

However it's still odd that none of the builders caught the error: https://lab.llvm.org/buildbot/#/changes/106242

fmorac added a comment.EditedAug 24 2023, 11:46 AM

Ok, the following patch also does the trick:

diff --git a/mlir/include/mlir/Target/LLVMIR/Dialect/All.h b/mlir/include/mlir/Target/LLVMIR/Dialect/All.h
index 895a37a63e9f..c25822c1c82b 100644
--- a/mlir/include/mlir/Target/LLVMIR/Dialect/All.h
+++ b/mlir/include/mlir/Target/LLVMIR/Dialect/All.h
@@ -60,6 +60,18 @@ static inline void registerAllToLLVMIRTranslations(DialectRegistry &registry) {
   ROCDL::registerROCDLTargetInterfaceExternalModels(registry);
 }
 
+/// Registers all the translations to LLVM IR required by GPU passes.
+/// TODO: Remove this function when a safe dialect interface registration
+/// mechanism is implemented, see D157703.
+static inline void
+registerAllGPUToLLVMIRTranslations(DialectRegistry &registry) {
+  registerBuiltinDialectTranslation(registry);
+  registerGPUDialectTranslation(registry);
+  registerLLVMDialectTranslation(registry);
+  registerNVVMDialectTranslation(registry);
+  registerROCDLDialectTranslation(registry);
+}
+
 /// Registers all dialects that can be translated from LLVM IR and the
 /// corresponding translation interfaces.
 static inline void
diff --git a/mlir/tools/mlir-opt/mlir-opt.cpp b/mlir/tools/mlir-opt/mlir-opt.cpp
index b47634476c62..a8aeffec1ae7 100644
--- a/mlir/tools/mlir-opt/mlir-opt.cpp
+++ b/mlir/tools/mlir-opt/mlir-opt.cpp
@@ -276,7 +276,7 @@ int main(int argc, char **argv) {
   DialectRegistry registry;
   registerAllDialects(registry);
   registerAllExtensions(registry);
-  registerAllToLLVMIRTranslations(registry);
+  registerAllGPUToLLVMIRTranslations(registry);
 
 #ifdef MLIR_INCLUDE_TESTS
   ::test::registerTestDialect(registry);

I still don't fully get why the bots didn't catch the error. If you can change this diff to the above patch I think it would be better solution, as it doesn't requires additional linking.

dtzWill updated this revision to Diff 553458.Aug 25 2023, 6:45 AM

Only pull in required translations, instead of linking more.

Is a bot broken? What kind of config do you use?

I'm seeing this in a downstream using an older clang/lld. Not sure why it is triggering:

ld.lld: error: undefined symbol: mlir::registerArmNeonDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)

ld.lld: error: undefined symbol: mlir::registerAMXDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)

ld.lld: error: undefined symbol: mlir::registerArmSMEDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)

ld.lld: error: undefined symbol: mlir::registerArmSVEDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)

ld.lld: error: undefined symbol: mlir::registerOpenACCDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)

ld.lld: error: undefined symbol: mlir::registerOpenMPDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)

ld.lld: error: undefined symbol: mlir::registerX86VectorDialectTranslation(mlir::DialectRegistry&)
>>> referenced by mlir-opt.cpp
>>>               tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(main)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
stellaraccident accepted this revision.Aug 25 2023, 2:22 PM

I'm +1 on landing this as it seems to restore the original behavior (I would assume based on some transitive linking fragility stemming from here: https://github.com/llvm/llvm-project/commit/7c4e8c6a273f25b3ef33e9c123b3969632ab59bb, but I haven't bisected for real).

This revision is now accepted and ready to land.Aug 25 2023, 2:22 PM
This revision was automatically updated to reflect the committed changes.