[mlir] Workaround for export lib generation on Windows for `mlir_arm_sme_abi_stubs` #73147

Hardcode84 · 2023-11-22T17:30:30Z

Using mlir cmake in downstream project fails with error

CMake Error at D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake:2537 (message):
  The imported target "mlir_arm_sme_abi_stubs" references the file

     "D:/projs/llvm/llvm-install/lib/mlir_arm_sme_abi_stubs.lib"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRConfig.cmake:37 (include)
  mlir/CMakeLists.txt:5 (find_package)

Windows cmake needs export libaries but it seems they are only being generated if you have at least one exported symbol.
Add export attributes to symbols.

Not sure what the best approach to fix this (probably we should just disable this lib on windows entirely), but it fixed things for me locally.

llvmbot · 2023-11-22T17:31:01Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-execution-engine

Author: Ivan Butygin (Hardcode84)

Changes

Using mlir cmake in downstream project fails with error

CMake Error at D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake:2537 (message):
  The imported target "mlir_arm_sme_abi_stubs" references the file

     "D:/projs/llvm/llvm-install/lib/mlir_arm_sme_abi_stubs.lib"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRConfig.cmake:37 (include)
  mlir/CMakeLists.txt:5 (find_package)

Windows cmake needs export libaries but it seems they are only being generated if you have at least one exported symbol. Add a dummy symbol to lib (export macros are copied from other mlir runnner libs).

Not sure what a best approach to fix this (probably we should just disable this lib on windows entirely), but it fixed things for me locally.

Full diff: https://github.com/llvm/llvm-project/pull/73147.diff

2 Files Affected:

(modified) mlir/lib/ExecutionEngine/ArmSMEStubs.cpp (+19)
(modified) mlir/lib/ExecutionEngine/CMakeLists.txt (+1)

diff --git a/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp b/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
index f9f64ad5e5ac81c..66df7900aeb9c53 100644
--- a/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
+++ b/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
@@ -10,6 +10,21 @@
 #include <cstdint>
 #include <iostream>
 
+#ifdef _WIN32
+#ifndef MLIR_RUNNERUTILS_EXPORT
+#ifdef mlir_arm_sme_abi_stubs_EXPORTS
+// We are building this library
+#define MLIR_ARNSMEABISTUBS_EXPORT __declspec(dllexport)
+#else
+// We are using this library
+#define MLIR_ARNSMEABISTUBS_EXPORT __declspec(dllimport)
+#endif // mlir_runner_utils_EXPORTS
+#endif // MLIR_RUNNERUTILS_EXPORT
+#else
+// Non-windows: use visibility attributes.
+#define MLIR_ARNSMEABISTUBS_EXPORT __attribute__((visibility("default")))
+#endif // _WIN32
+
 // The actual implementation of these routines is in:
 // compiler-rt/lib/builtins/aarch64/sme-abi.S. These stubs allow the current
 // ArmSME tests to run without depending on compiler-rt. This works as we don't
@@ -18,6 +33,10 @@
 // a path to an alternate implementation.
 
 extern "C" {
+void MLIR_ARNSMEABISTUBS_EXPORT _mlir_arm_sme_abi_stubs_exportstub() {
+  // TODO: is function is not used, need at least one exported symbol on Windows
+  // to generate export library.
+}
 
 bool LLVM_ATTRIBUTE_WEAK __aarch64_sme_accessible() {
   // The ArmSME tests are run within an emulator so we assume SME is available.
diff --git a/mlir/lib/ExecutionEngine/CMakeLists.txt b/mlir/lib/ExecutionEngine/CMakeLists.txt
index fe139661f2bbb5a..70c5a07ad1ab237 100644
--- a/mlir/lib/ExecutionEngine/CMakeLists.txt
+++ b/mlir/lib/ExecutionEngine/CMakeLists.txt
@@ -181,6 +181,7 @@ if(LLVM_ENABLE_PIC)
   add_mlir_library(mlir_arm_sme_abi_stubs
     SHARED
     ArmSMEStubs.cpp)
+  target_compile_definitions(mlir_arm_sme_abi_stubs PRIVATE mlir_arm_sme_abi_stubs_EXPORTS)
 
   if(MLIR_ENABLE_CUDA_RUNNER)
     # Configure CUDA support. Using check_language first allows us to give a

MacDue · 2023-11-22T18:21:14Z

mlir/lib/ExecutionEngine/ArmSMEStubs.cpp

+void MLIR_ARNSMEABISTUBS_EXPORT _mlir_arm_sme_abi_stubs_exportstub() {
+  // TODO: is function is not used, need at least one exported symbol on Windows
+  // to generate export library.
+}


Rather than add this dummy function, I think you can just add MLIR_ARNSMEABISTUBS_EXPORT to one (or all) of the functions already here.

This makes sense, although I can't test it immediately on non-windows, does those tests run on GH precommit runners?

They're not run as part of pre-commit, but this looks like it should be an NFC on Linux. I can give this patch a quick test tomorrow though.

…sme_abi_stubs` Using mlir smake in downstream project faild with error ``` CMake Error at D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake:2537 (message): The imported target "mlir_arm_sme_abi_stubs" references the file "D:/projs/llvm/llvm-install/lib/mlir_arm_sme_abi_stubs.lib" but this file does not exist. Possible reasons include: * The file was deleted, renamed, or moved to another location. * An install or uninstall procedure did not complete successfully. * The installation package was faulty and contained "D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake" but not all the files it references. Call Stack (most recent call first): D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRConfig.cmake:37 (include) mlir/CMakeLists.txt:5 (find_package) ``` Windows cmake needs export libaries but it seems they are only being generated if you have at least one exported symbol. Add a dummy symbol to lib (export macros is copied from other mlir runnner libs).

joker-eph · 2023-11-23T02:23:22Z

mlir/lib/ExecutionEngine/CMakeLists.txt

@@ -181,6 +181,7 @@ if(LLVM_ENABLE_PIC)
  add_mlir_library(mlir_arm_sme_abi_stubs
    SHARED
    ArmSMEStubs.cpp)
+  target_compile_definitions(mlir_arm_sme_abi_stubs PRIVATE mlir_arm_sme_abi_stubs_EXPORTS)


The case is surprising to me, is this something we do in the other libs?

To work on Windows, yes. There are some ways to tell it to indiscriminately export everything but I don't think we use that.

LLVM_ATTRIBUTE_WEAK is expanding to empty macro on Windows, which resulted in dll without any exported symbols, and export lib is not being generated for such dll (I don't know if it's general cmake behavior, llvm cmake or microsoft linker behavior). Adding __declspec(dllexport) fixed it. We don't see this for other libs because all of them export some symbols.

joker-eph

@stellaraccident knows this better than I do, but this does not strikes me as obviously wrong just now.

stellaraccident · 2023-11-23T02:37:36Z

mlir/lib/ExecutionEngine/CMakeLists.txt

@@ -181,6 +181,7 @@ if(LLVM_ENABLE_PIC)
  add_mlir_library(mlir_arm_sme_abi_stubs
    SHARED
    ArmSMEStubs.cpp)
+  target_compile_definitions(mlir_arm_sme_abi_stubs PRIVATE mlir_arm_sme_abi_stubs_EXPORTS)


To work on Windows, yes. There are some ways to tell it to indiscriminately export everything but I don't think we use that.

stellaraccident · 2023-11-23T02:43:38Z

mlir/lib/ExecutionEngine/ArmSMEStubs.cpp

+#endif // mlir_arm_sme_abi_stubs_EXPORTS
+#endif // MLIR_ARMSMEABISTUBS_EXPORT
+#else
+#define MLIR_ARMSMEABISTUBS_EXPORT LLVM_ATTRIBUTE_WEAK


To be fully resilient, you would also add __attribute__((visibility("default")), which would make this target compatible with -fvisiblity=hidden build modes (which gets used for a lot of deployment cases because it generates more efficient binaries).

I might suggest lining the terminology and style of ifdefs with how we do this for the CAPI: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir-c/Support.h#L46

I think:

#ifdef _WIN32 -> #if (defined(_WIN32) || defined(__CYGWIN__))

MLIR_ARMSMEABISTUBS_EXPORT -> MLIR_ARMSMEABISTUBS_EXPORTED

Since this is unconditionally/always a shared library, you can leave out the check for an equiv of MLIR_CAPI_ENABLE_WINDOWS_DLL_DECLSPEC.

MacDue

LGTM (aside from the outstanding comments) 👍

Hardcode84 · 2023-11-23T12:19:35Z

With __attribute__((visibility("default")) it's no longer NFC on linux, so let's hope it works )

…lir_arm_sme_abi_stubs` (#73147)" This reverts commit 6248c24. broke the bots

Hardcode84 · 2023-11-23T12:33:27Z

Of course it broke

Hardcode84 · 2023-11-23T12:34:50Z

Sorry, I'll leave __attribute__((visibility("default")) part for someone else, my only goal here is to fix win build.

…sme_abi_stubs` (llvm#73147) Using mlir cmake in downstream project fails with error ``` CMake Error at D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake:2537 (message): The imported target "mlir_arm_sme_abi_stubs" references the file "D:/projs/llvm/llvm-install/lib/mlir_arm_sme_abi_stubs.lib" but this file does not exist. Possible reasons include: * The file was deleted, renamed, or moved to another location. * An install or uninstall procedure did not complete successfully. * The installation package was faulty and contained "D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRTargets.cmake" but not all the files it references. Call Stack (most recent call first): D:/projs/llvm/llvm-install/lib/cmake/mlir/MLIRConfig.cmake:37 (include) mlir/CMakeLists.txt:5 (find_package) ``` Windows cmake needs export libaries but it seems they are only being generated if you have at least one exported symbol. Add export attributes to symbols. Not sure what the best approach to fix this (probably we should just disable this lib on windows entirely), but it fixed things for me locally.

…r_arm_sme_abi_stubs #73147 (#73238) #73147 Fixed the visibility macro

Hardcode84 requested review from c-rhodes, banach-space, joker-eph, MacDue and Dinistro November 22, 2023 17:30

llvmbot added mlir mlir:execution-engine labels Nov 22, 2023

MacDue reviewed Nov 22, 2023

View reviewed changes

Hardcode84 force-pushed the fix-armsme-exports branch from ef83df1 to 8d77b13 Compare November 22, 2023 18:28

fix typos

2ec01f9

joker-eph reviewed Nov 23, 2023

View reviewed changes

joker-eph approved these changes Nov 23, 2023

View reviewed changes

joker-eph requested a review from stellaraccident November 23, 2023 02:24

stellaraccident approved these changes Nov 23, 2023

View reviewed changes

MacDue approved these changes Nov 23, 2023

View reviewed changes

__attribute__((visibility("default")) and review comments

5b6db06

Hardcode84 merged commit 6248c24 into llvm:main Nov 23, 2023

Hardcode84 deleted the fix-armsme-exports branch November 23, 2023 12:23

Hardcode84 added a commit that referenced this pull request Nov 23, 2023

Revert "[mlir] Workaround for export lib generation on Windows for `m…

bf353a7

…lir_arm_sme_abi_stubs` (#73147)" This reverts commit 6248c24. broke the bots

Hardcode84 mentioned this pull request Nov 23, 2023

Reland [mlir] Workaround for export lib generation on Windows for mlir_arm_sme_abi_stubs #73147 #73238

Merged

Hardcode84 added a commit that referenced this pull request Nov 23, 2023

Reland [mlir] Workaround for export lib generation on Windows for mli…

0bda20b

…r_arm_sme_abi_stubs #73147 (#73238) #73147 Fixed the visibility macro

[mlir] Workaround for export lib generation on Windows for mlir_arm_sme_abi_stubs #73147

[mlir] Workaround for export lib generation on Windows for mlir_arm_sme_abi_stubs #73147

Uh oh!

Conversation

Hardcode84 commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MacDue Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hardcode84 Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joker-eph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MacDue left a comment

Choose a reason for hiding this comment

Uh oh!

Hardcode84 commented Nov 23, 2023

Uh oh!

Hardcode84 commented Nov 23, 2023

Uh oh!

Hardcode84 commented Nov 23, 2023

Uh oh!

Uh oh!

[mlir] Workaround for export lib generation on Windows for `mlir_arm_sme_abi_stubs` #73147

[mlir] Workaround for export lib generation on Windows for `mlir_arm_sme_abi_stubs` #73147

Hardcode84 commented Nov 22, 2023 •

edited

Loading

llvmbot commented Nov 22, 2023 •

edited

Loading

MacDue Nov 22, 2023 •

edited

Loading

Hardcode84 Nov 23, 2023 •

edited

Loading