Skip to content

Commit 057863a

Browse files
committed
[mlir] Fix build & test of mlir python bindings on Windows
There are a couple of issues with the python bindings on Windows: - `create_symlink` requires special permissions on Windows - using `copy_if_different` instead allows the build to complete and then be usable - the path to the `python_executable` is likely to contain spaces if python is installed in Program Files. llvm's python substitution adds extra quotes in order to account for this case, but mlir's own python substitution does not - the location of the shared libraries is different on windows - if the type is not specified for numpy arrays, they appear to be treated as strings I've implemented the smallest possible changes for each of these in the patch, but I would actually prefer a slightly more comprehensive fix for the python_executable and the shared libraries. For the python substitution, I think it makes sense to leverage the existing %python instead of adding %PYTHON and instead add a new variable for the case when preloading is needed. This would also make it clearer which tests are which and should be skipped on platforms where the preloading won't work. For the shared libraries, I think it would make sense to pass the correct path and extension (possibly even the names) to the python script since these are known by lit and don't have to be hardcoded in the test at all. Reviewed By: stellaraccident Differential Revision: https://reviews.llvm.org/D125122
1 parent a49d5e9 commit 057863a

File tree

4 files changed

+42
-17
lines changed

4 files changed

+42
-17
lines changed

mlir/cmake/modules/AddMLIRPython.cmake

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -229,12 +229,20 @@ function(add_mlir_python_modules name)
229229
get_filename_component(_install_path "${ARG_INSTALL_PREFIX}/${_dest_relative_path}" DIRECTORY)
230230

231231
file(MAKE_DIRECTORY "${_dest_dir}")
232+
233+
# On Windows create_symlink requires special permissions. Use copy_if_different instead.
234+
if(CMAKE_HOST_WIN32)
235+
set(_link_or_copy copy_if_different)
236+
else()
237+
set(_link_or_copy create_symlink)
238+
endif()
239+
232240
add_custom_command(
233241
TARGET ${modules_target} PRE_BUILD
234242
COMMENT "Copying python source ${_src_path} -> ${_dest_path}"
235243
DEPENDS "${_src_path}"
236244
BYPRODUCTS "${_dest_path}"
237-
COMMAND "${CMAKE_COMMAND}" -E create_symlink
245+
COMMAND "${CMAKE_COMMAND}" -E ${_link_or_copy}
238246
"${_src_path}" "${_dest_path}"
239247
)
240248
install(
@@ -285,7 +293,7 @@ function(add_mlir_python_modules name)
285293
endforeach()
286294

287295
# Create an install target.
288-
if (NOT LLVM_ENABLE_IDE)
296+
if(NOT LLVM_ENABLE_IDE)
289297
add_llvm_install_targets(
290298
install-${name}
291299
DEPENDS ${name}
@@ -483,10 +491,10 @@ function(add_mlir_python_extension libname extname)
483491
"INSTALL_COMPONENT;INSTALL_DIR;OUTPUT_DIRECTORY"
484492
"SOURCES;LINK_LIBS"
485493
${ARGN})
486-
if (ARG_UNPARSED_ARGUMENTS)
494+
if(ARG_UNPARSED_ARGUMENTS)
487495
message(FATAL_ERROR " Unhandled arguments to add_mlir_python_extension(${libname}, ... : ${ARG_UNPARSED_ARGUMENTS}")
488496
endif()
489-
if ("${ARG_SOURCES}" STREQUAL "")
497+
if("${ARG_SOURCES}" STREQUAL "")
490498
message(FATAL_ERROR " Missing SOURCES argument to add_mlir_python_extension(${libname}, ...")
491499
endif()
492500

@@ -527,12 +535,9 @@ function(add_mlir_python_extension libname extname)
527535
)
528536
endif()
529537

530-
# Python extensions depends *only* on the public API and LLVMSupport unless
531-
# if further dependencies are added explicitly.
532538
target_link_libraries(${libname}
533539
PRIVATE
534540
${ARG_LINK_LIBS}
535-
${PYEXT_LIBADD}
536541
)
537542

538543
target_link_options(${libname}
@@ -545,7 +550,7 @@ function(add_mlir_python_extension libname extname)
545550
################################################################################
546551
# Install
547552
################################################################################
548-
if (ARG_INSTALL_DIR)
553+
if(ARG_INSTALL_DIR)
549554
install(TARGETS ${libname}
550555
COMPONENT ${ARG_INSTALL_COMPONENT}
551556
LIBRARY DESTINATION ${ARG_INSTALL_DIR}

mlir/test/lit.cfg.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,10 @@
9494
# TODO: detect Darwin/Windows situation (or mark these tests as unsupported on these platforms).
9595
if "asan" in config.available_features and "Linux" in config.host_os:
9696
python_executable = f"LD_PRELOAD=$({config.host_cxx} -print-file-name=libclang_rt.asan-{config.host_arch}.so) {config.python_executable}"
97+
# On Windows the path to python could contains spaces in which case it needs to be provided in quotes.
98+
# This is the equivalent of how %python is setup in llvm/utils/lit/lit/llvm/config.py.
99+
elif "Windows" in config.host_os:
100+
python_executable = '"%s"' % (python_executable)
97101
tools.extend([
98102
ToolSubst('%PYTHON', python_executable, unresolved='ignore'),
99103
])

mlir/test/python/dialects/shape.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ def testConstShape():
2323
RankedTensorType.get((12, -1), f32))
2424
def const_shape_tensor(arg):
2525
return shape.ConstShapeOp(
26-
DenseElementsAttr.get(np.array([10, 20]), type=IndexType.get()))
26+
DenseElementsAttr.get(np.array([10, 20], dtype=np.int64), type=IndexType.get()))
2727

2828
# CHECK-LABEL: func @const_shape_tensor(%arg0: tensor<12x?xf32>)
2929
# CHECK: shape.const_shape [10, 20] : tensor<2xindex>

mlir/test/python/execution_engine.py

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -345,13 +345,21 @@ def testSharedLibLoad():
345345
arg0_memref_ptr = ctypes.pointer(
346346
ctypes.pointer(get_ranked_memref_descriptor(arg0)))
347347

348+
if sys.platform == 'win32':
349+
shared_libs = [
350+
"../../../../bin/mlir_runner_utils.dll",
351+
"../../../../bin/mlir_c_runner_utils.dll"
352+
]
353+
else:
354+
shared_libs = [
355+
"../../../../lib/libmlir_runner_utils.so",
356+
"../../../../lib/libmlir_c_runner_utils.so"
357+
]
358+
348359
execution_engine = ExecutionEngine(
349360
lowerToLLVM(module),
350361
opt_level=3,
351-
shared_libs=[
352-
"../../../../lib/libmlir_runner_utils.so",
353-
"../../../../lib/libmlir_c_runner_utils.so"
354-
])
362+
shared_libs=shared_libs)
355363
execution_engine.invoke("main", arg0_memref_ptr)
356364
# CHECK: Unranked Memref
357365
# CHECK-NEXT: [42]
@@ -379,13 +387,21 @@ def testNanoTime():
379387
func.func private @printMemrefI64(memref<*xi64>) attributes { llvm.emit_c_interface }
380388
}""")
381389

390+
if sys.platform == 'win32':
391+
shared_libs = [
392+
"../../../../bin/mlir_runner_utils.dll",
393+
"../../../../bin/mlir_c_runner_utils.dll"
394+
]
395+
else:
396+
shared_libs = [
397+
"../../../../lib/libmlir_runner_utils.so",
398+
"../../../../lib/libmlir_c_runner_utils.so"
399+
]
400+
382401
execution_engine = ExecutionEngine(
383402
lowerToLLVM(module),
384403
opt_level=3,
385-
shared_libs=[
386-
"../../../../lib/libmlir_runner_utils.so",
387-
"../../../../lib/libmlir_c_runner_utils.so"
388-
])
404+
shared_libs=shared_libs)
389405
execution_engine.invoke("main")
390406
# CHECK: Unranked Memref
391407
# CHECK: [{{.*}}]

0 commit comments

Comments
 (0)