Skip to content
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

Windows native port #2478

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

Windows native port #2478

wants to merge 53 commits into from

Conversation

gshimansky
Copy link
Contributor

@gshimansky gshimansky commented Oct 11, 2024

Fixes #2407. Current state is that code builds on windows and is able to pass many unit tests. More fixes will be done when this patch is integrated into main branch.

@whitneywhtsang
Copy link
Contributor

Please fix the CI failures. And can we add testing to prevent regression?

@gshimansky
Copy link
Contributor Author

Please fix the CI failures. And can we add testing to prevent regression?

Sure, we will add windows CI too but at the moment we don't have almost any windows infrastructure, and we need to figure out how to do automation. All of the bash scripts are not going to run on windows.

@etiotto etiotto requested review from a team and alexbaden October 15, 2024 14:57
@gshimansky gshimansky force-pushed the gregory/windows-support branch 2 times, most recently from d1baf6d to 45d26b3 Compare October 15, 2024 22:13
@gshimansky
Copy link
Contributor Author

CI is green on Linux so PR is ready for review now.

Copy link
Contributor

@victor-eds victor-eds left a comment

Choose a reason for hiding this comment

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

Is this intended to be upstreamed?

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
bin/RegisterTritonDialects.h Outdated Show resolved Hide resolved
python/test/unit/language/test_core.py Outdated Show resolved Hide resolved
python/test/unit/language/test_core.py Outdated Show resolved Hide resolved
python/test/unit/language/print_helper.py Outdated Show resolved Hide resolved
python/test/unit/language/test_reproducer.py Outdated Show resolved Hide resolved
python/test/unit/language/test_compile_errors.py Outdated Show resolved Hide resolved
python/triton/compiler/compiler.py Outdated Show resolved Hide resolved
python/triton/compiler/compiler.py Outdated Show resolved Hide resolved
@gshimansky gshimansky force-pushed the gregory/windows-support branch 3 times, most recently from 36d0abf to 705730c Compare October 18, 2024 14:47
python/setup.py Outdated Show resolved Hide resolved
wkpark added a commit to wkpark/triton that referenced this pull request Oct 19, 2024
 * based on Windows support PR triton-lang#2465 by @andreigh
   - triton-lang#2465
 * manually applied, rebased, fix lint errors
 * use sysconfig.get_config_var() to get the path of python*.lib
 * clang fix for windows
 * remove '-fPIC' for windows clang
 * fix download_and_copy() to support windows
 * add "exe" extension for windows
 * use "pyd" extension for windows to make importlib work
 * third_party/nvidia: fix for windows
 * win32 fix _path_to_binary()
 * add library_dir, include_dir for win32
 * backend/compiler lazy remove temp files to support windows
 * additional works done by @mantaionut (2024/05/31)
 * rework for latest triton and cleanup (2024/10/14)
 * extract minimal fixes to support win32+clang (2024/10/16)
 * get exe/so extension using sysconfig (suggested by @anmyachev)

see also:
 intel/intel-xpu-backend-for-triton#2478

Original-author-by: Andrei Gheorghe <andrei@dharmaventures.co>
Signed-off-by: Won-Kyu Park <wkpark@gmail.com>
wkpark added a commit to wkpark/triton that referenced this pull request Oct 19, 2024
 * based on Windows support PR triton-lang#2465 by @andreigh
   - triton-lang#2465
 * manually applied, rebased, fix lint errors
 * remove '/A' platform option to use ninja
 * use sysconfig.get_config_var() to get the path of python*.lib
 * clang fix for windows
 * remove '-fPIC' for windows clang
 * fix download_and_copy() to support windows
 * add "exe" extension for windows
 * use "pyd" extension for windows to make importlib work
 * third_party/nvidia: fix for windows
 * win32 fix _path_to_binary()
 * add library_dir, include_dir for win32
 * backend/compiler lazy remove temp files to support windows
 * additional works done by @mantaionut (2024/05/31)
 * rework for latest triton and cleanup (2024/10/14)
 * extract minimal fixes to support win32+clang (2024/10/16)
 * get exe/so extension using sysconfig (suggested by @anmyachev)

see also:
 intel/intel-xpu-backend-for-triton#2478

Original-author-by: Andrei Gheorghe <andrei@dharmaventures.co>
Signed-off-by: Won-Kyu Park <wkpark@gmail.com>
wkpark added a commit to wkpark/triton that referenced this pull request Oct 19, 2024
 * based on Windows support PR triton-lang#2465 by @andreigh
   - triton-lang#2465
 * manually applied, rebased, fix lint errors
 * remove '/A' platform option to use ninja
 * use sysconfig.get_config_var() to get the path of python*.lib
 * clang fix for windows
 * remove '-fPIC' for windows clang
 * fix download_and_copy() to support windows
 * add "exe" extension for windows
 * use "pyd" extension for windows to make importlib work
 * third_party/nvidia: fix for windows
 * win32 fix _path_to_binary()
 * add library_dir, include_dir for win32
 * backend/compiler lazy remove temp files to support windows
 * additional works done by @mantaionut (2024/05/31)
 * rework for latest triton and cleanup (2024/10/14)
 * extract minimal fixes to support win32+clang (2024/10/16)
 * get exe/so extension using sysconfig (suggested by @anmyachev)

see also:
 intel/intel-xpu-backend-for-triton#2478

Original-author-by: Andrei Gheorghe <andrei@dharmaventures.co>
Signed-off-by: Won-Kyu Park <wkpark@gmail.com>
wkpark added a commit to wkpark/triton that referenced this pull request Oct 19, 2024
 * based on Windows support PR triton-lang#2465 by @andreigh
   - triton-lang#2465
 * manually applied, rebased, fix lint errors
 * remove '/A' platform option to use ninja
 * use sysconfig.get_config_var() to get the path of python*.lib
 * clang fix for windows
 * remove '-fPIC' for windows clang
 * fix download_and_copy() to support windows
 * add "exe" extension for windows
 * use "pyd" extension for windows to make importlib work
 * third_party/nvidia: fix for windows
 * win32 fix _path_to_binary()
 * add library_dir, include_dir for win32
 * backend/compiler lazy remove temp files to support windows
 * additional works done by @mantaionut (2024/05/31)
 * rework for latest triton and cleanup (2024/10/14)
 * extract minimal fixes to support win32+clang (2024/10/16)
 * get exe/so extension using sysconfig (suggested by @anmyachev)

see also:
 intel/intel-xpu-backend-for-triton#2478

Original-author-by: Andrei Gheorghe <andrei@dharmaventures.co>
Signed-off-by: Won-Kyu Park <wkpark@gmail.com>
bin/CMakeLists.txt Outdated Show resolved Hide resolved
python/setup.py Outdated Show resolved Hide resolved
python/src/main.cc Outdated Show resolved Hide resolved
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
return None


def find_visual_studio(version_ranges):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we reuse code from CLFinder.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can and it should work in theory, but it doesn't. For some reason when I import these functions from CLFinder I am getting an error LINK : fatal error LNK1168: cannot open C:\b\tr\python\triton\_C\libtriton.pyd for writing. I have no idea how they are related but this link error is stably reproducible for me. I tried to debug the problem and found that environment after calling set_env_vars is identical when functions are reused from CLFinder.py or setup.py has its own copies, so now I am out of ideas how libtryton.pyd may end up locked.

anmyachev added a commit that referenced this pull request Nov 4, 2024
…:getSubOpSize` for portability between platforms (#2620)

Part of #2478. To reduce diff.

---------

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Co-authored-by: Ettore Tiotto <ettore.tiotto@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
python/setup.py Outdated Show resolved Hide resolved
raise FileNotFoundError("vswhere.exe not found.")

for version_range in version_ranges:
command = [
Copy link
Contributor

Choose a reason for hiding this comment

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

For me it works only if I specify -products:
"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe" -version "[17.0,18.0)" -products Microsoft.VisualStudio.Product.BuildTools -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath -prerelease

@gshimansky do you know why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it works just fine if I don't specify -products or if I specify -products "*". Specifying -products Microsoft.VisualStudio.Product.BuildTools doesn't find anything for me but specifying -products Microsoft.VisualStudio.Product.Professional does.

Copy link
Contributor

Choose a reason for hiding this comment

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

-products "*" works for me too. It seems that this is because I did not install the entire studio, but only the build tools. Should we add -products "*" to allow this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know whether it may result in multiple different products found as the result, but we can add it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether it may result in multiple different products found as the result, but we can add it for now.

I thought about that too. But couldn't it be the same default behavior when -products parameter is not specified at all?

anmyachev added a commit that referenced this pull request Nov 6, 2024
….py` (#2637)

Part of #2478

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Comment on lines +181 to +182
} \
symbolName##_t funcHandle = \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we explicitly clear any existing error on Windows as well?

Suggested change
} \
symbolName##_t funcHandle = \
} \
/* Clear any existing error */ \
SetLastError(NO_ERROR); \
symbolName##_t funcHandle = \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that is needed. GetLastError is used for information purposes only (think of it as of errno in POSIX), any next call of Win32 API should set its own error status, clear it if it is successful.

Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
@@ -194,8 +194,18 @@ static PyObject *loadBinary(PyObject *self, PyObject *args) {
const size_t binary_size = PyBytes_Size(py_bytes);

uint8_t *binary_ptr = (uint8_t *)PyBytes_AsString(py_bytes);
const auto ctx =
sycl_device.get_platform().ext_oneapi_get_default_context();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked this and it does work. But I am not sure it is a good way because triton on Windows will work only with this variable set to 1. Even on Linux if someone has environment SYCL_ENABLE_DEFAULT_CONTEXTS=0 we're going to throw an uncaught exception, are you sure it is ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PoC] Windows native Triton XPU build
6 participants