-
Notifications
You must be signed in to change notification settings - Fork 536
Windows x64 Build with support for xnnpack and llama example #6979
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/6979
Note: Links to docs will display an error until the docs builds have been completed. ❌ 6 New FailuresAs of commit 8e2b8bd with merge base 3475707 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
- fix extension/data_loader/file_data_loader.cpp for Windows x64
❌ 🤖 pytorchbot command failed:
Try |
@pytorchbot label "Release notes: build" |
Didn't find following labels among repository labels: Release notes: build |
@pytorchbot label "release notes: build" |
Hi @kirklandsign and @dbort! Could you kindly help assign reviewers to this PR? Many thanks! |
- attempt to fix extension/data_loader/file_data_loader.cpp:20:10: fatal error: 'executorch/runtime/platform/unistd.h' file not found
Hi @dbort I attempted to address the issues in the checks, could you kindly help rerun them? Thanks 😄 |
…E for executorch in Windows
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.
IMO this change would benefit from splitting it into several separate PRs, namely:
- Please rename
unistd.h
tocompat_unistd.h
as redefining system headers might create some hard to debug confusions - Submit a separate PR for adding dataloader on Windows
- Submit smaller fixes for individual changes in headers/cpp files explaining why they are needed
- Submit a separate PR with generic build system changes
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.
XNN related changes look good to me
endif() | ||
|
||
|
||
if (WIN32) |
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.
this looks fine to me, i'm assuming this has to be done for every flatbuffer outputted header file
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.
this looks fine to me, i'm assuming this has to be done for every flatbuffer outputted header file
Thanks for the review! As suggested, we will split into smaller PRs (#7217), and apply these "platform-dependent shell command" changes everywhere in a later PR
// There currently doesn't seem to be a great way to do this in Windows and | ||
// given that weak linkage is not really critical on Windows, we'll just leave | ||
// it as a stub. | ||
#define ET_WEAK |
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.
@vortex-captain does this work with MSVC now? Is there a certain version of MSVC this works for, because i remember it didn't work for the older versions.
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.
@vortex-captain does this work with MSVC now? Is there a certain version of MSVC this works for, because i remember it didn't work for the older versions.
yes, Windows x64 build uses clang-cl compiler, which supports __attribute__((weak))
@@ -0,0 +1,251 @@ | |||
/* | |||
* Copyright (c) Google Inc. and affiliates. |
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.
I think as @malfet mentioned, isolating these changes to a separate PR would be much more easier to review.
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.
@@ -0,0 +1,75 @@ | |||
/* |
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.
I feel like we should maybe create a separate folder for this like runtime/platform/windows
and put this under there. @dbort what do you think?
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.
I feel like we should maybe create a separate folder for this like
runtime/platform/windows
and put this under there. @dbort what do you think?
As @malfet suggested, We renamed unistd.h
to compat_unistd.h
in the new PR which would serve as a wrapper of <unistd.h>
used in different platforms. Would it be clearer to put it in runtime/platform/windows
? Thanks!
Thanks for the great suggestions! We plan to split this PR into 3 smaller ones, please find details in the new PR here, in which |
Closing this PR now, as the changes contained in this PR are now merged as a part of the recent Windows support effort i.e. #9198 |
Summary
This PR introduces support for out-of-the-box builds on Windows x64 with xnnpack, simplifying the setup process for developers using executorch on Windows. Additionally, it lays the groundwork for future Windows build pipelines and prebuilt PyPI packages.
Fixes #4661
Test plan
PR tested by running minimal
executor_runner
example (https://pytorch.org/executorch/stable/getting-started-setup.html#run-your-program) in the following environment:Steps:
References
#4681
#5252
#4899
#5164
#4993