Fix data loader build on Windows, re-enable pybind job #13564
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,16 +21,28 @@ set_overridable_option(EXECUTORCH_BUILD_EXTENSION_FLAT_TENSOR ON) | |
| set_overridable_option(EXECUTORCH_BUILD_EXTENSION_DATA_LOADER ON) | ||
| set_overridable_option(EXECUTORCH_BUILD_KERNELS_OPTIMIZED ON) | ||
| set_overridable_option(EXECUTORCH_BUILD_EXTENSION_MODULE ON) | ||
|
|
||
| if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") | ||
| set_overridable_option(EXECUTORCH_BUILD_COREML ON) | ||
| set_overridable_option(EXECUTORCH_BUILD_EXTENSION_TRAINING ON) | ||
| elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
| set_overridable_option(EXECUTORCH_BUILD_COREML ON) | ||
| set_overridable_option(EXECUTORCH_BUILD_EXTENSION_TRAINING ON) | ||
| elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows" OR CMAKE_SYSTEM_NAME STREQUAL | ||
| "WIN32" | ||
| ) | ||
| if(NOT CMAKE_GENERATOR_TOOLSET MATCHES "ClangCL") | ||
| message( | ||
| FATAL_ERROR | ||
| "ExecuTorch requires the ClangCL toolset on Windows. Please configure with -T ClangCL." | ||
| ) | ||
| endif() | ||
|
|
||
| # These XNNPACK options don't currently build on Windows with Clang. | ||
|
There was a problem hiding this comment. Where do these magic incantations come from There was a problem hiding this comment. These are 4 optional XNNPACK features (exposed by XNNPACK's CMakeLists) that clang-cl has issues with due to the /vlen compiler argument. These disable a small subset of the available kernels, though they are relatively niche and should have minimal perf impact. It should be fixed or gated upstream in XNNPACK itself. I intend to file an issue there and maybe just fix it myself. They have some gating logic already (https://github.com/google/XNNPACK/blob/923253023555f75c38d96511aa7fa59b9ef9c25c/CMakeLists.txt#L176) I could also move these into the XNNPACK backend CMakeLists, rather than the presets, as they are functionally an internal detail, and it would also ensure that they are always disabled on Windows. There was a problem hiding this comment. Thinking through it more, I think I will move these into the XNNPACK backend CMakeLists, rather than the preset. |
||
| set_overridable_option(XNNPACK_ENABLE_AVX256SKX OFF) | ||
| set_overridable_option(XNNPACK_ENABLE_AVX256VNNI OFF) | ||
| set_overridable_option(XNNPACK_ENABLE_AVX256VNNIGFNI OFF) | ||
| set_overridable_option(XNNPACK_ENABLE_AVX512BF16 OFF) | ||
| else() | ||
| message( | ||
| FATAL_ERROR "Unsupported CMAKE_SYSTEM_NAME for pybind: ${CMAKE_SYSTEM_NAME}" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? I thought scott already selectively included this source file in build whatever.bzl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not. I might be thinking of selectively removing the header from the public include if not on windows or something
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I chatted with Scott briefly on this. His recommendation was to manage it locally. The main build variables list has to be mechanically parsable from both CMake and Buck, so it can't include more complex conditional logic.