Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 23, 2023

Conversation

Hardcode84
Copy link
Contributor

@Hardcode84 Hardcode84 commented Nov 23, 2023

#73147

Fixed the visibility macro

@llvmbot
Copy link
Member

llvmbot commented Nov 23, 2023

@llvm/pr-subscribers-mlir

Author: Ivan Butygin (Hardcode84)

Changes

#73147

Remove __attribute__((visibility("default")) for now as it caused strange failure.


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

2 Files Affected:

  • (modified) mlir/lib/ExecutionEngine/ArmSMEStubs.cpp (+19-5)
  • (modified) mlir/lib/ExecutionEngine/CMakeLists.txt (+1)
diff --git a/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp b/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
index f9f64ad5e5ac81c..1861776d6a0ba5c 100644
--- a/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
+++ b/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
@@ -10,6 +10,20 @@
 #include <cstdint>
 #include <iostream>
 
+#if (defined(_WIN32) || defined(__CYGWIN__))
+#ifndef MLIR_ARMSMEABISTUBS_EXPORTED
+#ifdef mlir_arm_sme_abi_stubs_EXPORTS
+// We are building this library
+#define MLIR_ARMSMEABISTUBS_EXPORTED __declspec(dllexport)
+#else
+// We are using this library
+#define MLIR_ARMSMEABISTUBS_EXPORTED __declspec(dllimport)
+#endif // mlir_arm_sme_abi_stubs_EXPORTS
+#endif // MLIR_ARMSMEABISTUBS_EXPORTED
+#else
+#define MLIR_ARMSMEABISTUBS_EXPORTED LLVM_ATTRIBUTE_WEAK
+#endif // (defined(_WIN32) || defined(__CYGWIN__))
+
 // 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
@@ -19,7 +33,7 @@
 
 extern "C" {
 
-bool LLVM_ATTRIBUTE_WEAK __aarch64_sme_accessible() {
+bool MLIR_ARMSMEABISTUBS_EXPORTED __aarch64_sme_accessible() {
   // The ArmSME tests are run within an emulator so we assume SME is available.
   return true;
 }
@@ -29,20 +43,20 @@ struct sme_state {
   int64_t x1;
 };
 
-sme_state LLVM_ATTRIBUTE_WEAK __arm_sme_state() {
+sme_state MLIR_ARMSMEABISTUBS_EXPORTED __arm_sme_state() {
   std::cerr << "[warning] __arm_sme_state() stubbed!\n";
   return sme_state{};
 }
 
-void LLVM_ATTRIBUTE_WEAK __arm_tpidr2_restore() {
+void MLIR_ARMSMEABISTUBS_EXPORTED __arm_tpidr2_restore() {
   std::cerr << "[warning] __arm_tpidr2_restore() stubbed!\n";
 }
 
-void LLVM_ATTRIBUTE_WEAK __arm_tpidr2_save() {
+void MLIR_ARMSMEABISTUBS_EXPORTED __arm_tpidr2_save() {
   std::cerr << "[warning] __arm_tpidr2_save() stubbed!\n";
 }
 
-void LLVM_ATTRIBUTE_WEAK __arm_za_disable() {
+void MLIR_ARMSMEABISTUBS_EXPORTED __arm_za_disable() {
   std::cerr << "[warning] __arm_za_disable() stubbed!\n";
 }
 }
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

@llvmbot
Copy link
Member

llvmbot commented Nov 23, 2023

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

Author: Ivan Butygin (Hardcode84)

Changes

#73147

Remove __attribute__((visibility("default")) for now as it caused strange failure.


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

2 Files Affected:

  • (modified) mlir/lib/ExecutionEngine/ArmSMEStubs.cpp (+19-5)
  • (modified) mlir/lib/ExecutionEngine/CMakeLists.txt (+1)
diff --git a/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp b/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
index f9f64ad5e5ac81c..1861776d6a0ba5c 100644
--- a/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
+++ b/mlir/lib/ExecutionEngine/ArmSMEStubs.cpp
@@ -10,6 +10,20 @@
 #include <cstdint>
 #include <iostream>
 
+#if (defined(_WIN32) || defined(__CYGWIN__))
+#ifndef MLIR_ARMSMEABISTUBS_EXPORTED
+#ifdef mlir_arm_sme_abi_stubs_EXPORTS
+// We are building this library
+#define MLIR_ARMSMEABISTUBS_EXPORTED __declspec(dllexport)
+#else
+// We are using this library
+#define MLIR_ARMSMEABISTUBS_EXPORTED __declspec(dllimport)
+#endif // mlir_arm_sme_abi_stubs_EXPORTS
+#endif // MLIR_ARMSMEABISTUBS_EXPORTED
+#else
+#define MLIR_ARMSMEABISTUBS_EXPORTED LLVM_ATTRIBUTE_WEAK
+#endif // (defined(_WIN32) || defined(__CYGWIN__))
+
 // 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
@@ -19,7 +33,7 @@
 
 extern "C" {
 
-bool LLVM_ATTRIBUTE_WEAK __aarch64_sme_accessible() {
+bool MLIR_ARMSMEABISTUBS_EXPORTED __aarch64_sme_accessible() {
   // The ArmSME tests are run within an emulator so we assume SME is available.
   return true;
 }
@@ -29,20 +43,20 @@ struct sme_state {
   int64_t x1;
 };
 
-sme_state LLVM_ATTRIBUTE_WEAK __arm_sme_state() {
+sme_state MLIR_ARMSMEABISTUBS_EXPORTED __arm_sme_state() {
   std::cerr << "[warning] __arm_sme_state() stubbed!\n";
   return sme_state{};
 }
 
-void LLVM_ATTRIBUTE_WEAK __arm_tpidr2_restore() {
+void MLIR_ARMSMEABISTUBS_EXPORTED __arm_tpidr2_restore() {
   std::cerr << "[warning] __arm_tpidr2_restore() stubbed!\n";
 }
 
-void LLVM_ATTRIBUTE_WEAK __arm_tpidr2_save() {
+void MLIR_ARMSMEABISTUBS_EXPORTED __arm_tpidr2_save() {
   std::cerr << "[warning] __arm_tpidr2_save() stubbed!\n";
 }
 
-void LLVM_ATTRIBUTE_WEAK __arm_za_disable() {
+void MLIR_ARMSMEABISTUBS_EXPORTED __arm_za_disable() {
   std::cerr << "[warning] __arm_za_disable() stubbed!\n";
 }
 }
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

@Hardcode84
Copy link
Contributor Author

@MacDue can you please test it on your side

@Hardcode84
Copy link
Contributor Author

Hardcode84 commented Nov 23, 2023

oh, nvm, I know what happend, I messed the macro (but it wasn't caught in precommit for some reason)

Copy link
Member

@MacDue MacDue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds fine to me locally 👍

…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.
@Hardcode84 Hardcode84 merged commit 0bda20b into llvm:main Nov 23, 2023
@Hardcode84 Hardcode84 deleted the arm-sme-again branch November 23, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants