[Mlir-commits] [mlir] 784a5bc - [mlir] Fix python bindings build on Windows in Debug

Stella Stamenova llvmlistbot at llvm.org
Mon May 9 19:47:54 PDT 2022 

Author: Stella Stamenova
Date: 2022-05-09T19:46:47-07:00
New Revision: 784a5bccfd2bffd698be18ad13cdcd2162c981e2

URL: https://github.com/llvm/llvm-project/commit/784a5bccfd2bffd698be18ad13cdcd2162c981e2
DIFF: https://github.com/llvm/llvm-project/commit/784a5bccfd2bffd698be18ad13cdcd2162c981e2.diff

LOG: [mlir] Fix python bindings build on Windows in Debug

Currently, building mlir with the python bindings enabled on Windows in Debug is broken because pybind11, python and cmake don't like to play together. This change normalizes how the three interact, so that the builds can now run and succeed.

The main issue is that python and cmake both make assumptions about which libraries are needed in a Windows build based on the flavor.
- cmake assumes that a debug (or a debug-like) flavor of the build will always require pythonX_d.lib and provides no option/hint to tell it to use a different library. cmake does find both the debug and release versions, but then uses the debug library.
- python (specifically pyconfig.h and by extension python.h) hardcodes the dependency on pythonX_d.lib or pythonX.lib depending on whether `_DEBUG` is defined. This is NOT transparent - it does not show up anywhere in the build logs until the link step fails with `pythonX_d.lib is missing` (or `pythonX.lib is missing`)
- pybind11 tries to "fix" this by implementing a workaround - unless Py_DEBUG is defined, `_DEBUG` is explicitly undefined right before including python headers. This also requires some windows headers to be included differently, so while clever, this is a non-trivial workaround.

mlir itself includes the pybind11 headers (which contain the workaround) AS WELL AS python.h, essentially always requiring both pythonX.lib and pythonX_d.lib for linking. cmake explicitly only adds one or the other, so the build fails.

This change does a couple of things:
- In the cmake files, explicitly add the release version of the python library on Windows builds regardless of flavor. Since Py_DEBUG is not defined, pybind11 will always require release and it will be satisfied
- To satisfy python as well, this change removes any explicit inclusions of Python.h on Windows instead relying on the fact that pybind11 headers will bring in what is needed

There are a few additional things that we could do but I rejected as unnecessary at this time:
- define Py_DEBUG based on the CMAKE_BUILD_TYPE - this will *mostly* work, we'd have to think through multiconfig generators like VS, but it's possible. There doesn't seem to be a need to link against debug python at the moment, so I chose not to overcomplicate the build and always default to release
- similar to above, but define Py_DEBUG based on the CMAKE_BUILD_TYPE *as well as* the presence of the debug python library (`Python3_LIBRARY_DEBUG`). Similar to above, this seems unnecessary right now. I think it's slightly better than above because most people don't actually have the debug version of python installed, so this would prevent breaks in that case.
- similar to the two above, but add a cmake variable to control the logic
- implement the pybind11 workaround directly in mlir (specifically in Interop.h) so that Python.h can still be included directly. This seems prone to error and a pain to maintain in lock step with pybind11
- reorganize how the pybind11 headers are included and place at least one of them in Interop.h directly, so that the header has all of its dependencies included as was the original intention. I decided against this because it really doesn't need pybind11 logic and it's always included after pybind11 is, so we don't necessarily need the python includes

Reviewed By: stellaraccident

Differential Revision: https://reviews.llvm.org/D125284

Added: 
    

Modified: 
    mlir/cmake/modules/AddMLIRPython.cmake
    mlir/include/mlir-c/Bindings/Python/Interop.h
    mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
    mlir/lib/Bindings/Python/IRCore.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/cmake/modules/AddMLIRPython.cmake b/mlir/cmake/modules/AddMLIRPython.cmake
index ec558e1e64aba..8750b10c96371 100644
--- a/mlir/cmake/modules/AddMLIRPython.cmake
+++ b/mlir/cmake/modules/AddMLIRPython.cmake
@@ -547,6 +547,19 @@ function(add_mlir_python_extension libname extname)
       $<$<PLATFORM_ID:Linux>:LINKER:--exclude-libs,ALL>
   )
 
+  if(WIN32)
+    # On Windows, pyconfig.h (and by extension python.h) hardcode the version of the
+    # python library which will be used for linkage depending on the flavor of the build.
+    # pybind11 has a workaround which depends on the definition of Py_DEBUG (if Py_DEBUG
+    # is not passed in as a compile definition, pybind11 undefs _DEBUG when including
+    # python.h, so that the release python library would be used).
+    # Since mlir uses pybind11, we can leverage their workaround by never directly
+    # pyconfig.h or python.h and instead relying on the pybind11 headers to include the
+    # necessary python headers. This results in mlir always linking against the
+    # release python library via the (undocumented) cmake property Python3_LIBRARY_RELEASE.
+    target_link_libraries(${libname} PRIVATE ${Python3_LIBRARY_RELEASE})
+  endif()
+
   ################################################################################
   # Install
   ################################################################################

diff  --git a/mlir/include/mlir-c/Bindings/Python/Interop.h b/mlir/include/mlir-c/Bindings/Python/Interop.h
index 7efe8500af71d..86b57cd738f02 100644
--- a/mlir/include/mlir-c/Bindings/Python/Interop.h
+++ b/mlir/include/mlir-c/Bindings/Python/Interop.h
@@ -21,7 +21,17 @@
 #ifndef MLIR_C_BINDINGS_PYTHON_INTEROP_H
 #define MLIR_C_BINDINGS_PYTHON_INTEROP_H
 
+// We *should*, in theory, include Python.h here in order to import the correct
+// definitions for what we need below, however, importing Python.h directly on
+// Windows results in the enforcement of either pythonX.lib or pythonX_d.lib
+// depending on the build flavor. Instead, we rely on the fact that this file
+// (Interop.h) is always included AFTER pybind11 and will therefore have access
+// to the definitions from Python.h in addition to having a workaround applied
+// through the pybind11 headers that allows us to control which python library
+// is used.
+#if !defined(_MSC_VER)
 #include <Python.h>
+#endif
 
 #include "mlir-c/AffineExpr.h"
 #include "mlir-c/AffineMap.h"

diff  --git a/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp b/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
index 9016900185c6c..f5179bd7c6a09 100644
--- a/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
+++ b/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
@@ -6,7 +6,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "mlir-c/Bindings/Python/Interop.h"
 #include "mlir-c/ExecutionEngine.h"
 #include "mlir/Bindings/Python/PybindAdaptors.h"
 

diff  --git a/mlir/lib/Bindings/Python/IRCore.cpp b/mlir/lib/Bindings/Python/IRCore.cpp
index d1877a11b8154..37ca3b953067a 100644
--- a/mlir/lib/Bindings/Python/IRCore.cpp
+++ b/mlir/lib/Bindings/Python/IRCore.cpp
@@ -19,7 +19,6 @@
 #include "mlir-c/Registration.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
-#include <pybind11/stl.h>
 
 #include <utility>

More information about the Mlir-commits mailing list