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

adaption to Windows #151

Merged
merged 21 commits into from
Mar 4, 2024
Merged

adaption to Windows #151

merged 21 commits into from
Mar 4, 2024

Conversation

Thomas008
Copy link
Contributor

@Thomas008 Thomas008 commented Feb 1, 2024

clang is being used to generate directly the executable binary from the LLVM IR that is produced by GPUCompiler (instead of first generating an object file with help of GPUCompiler, and then using clang for linking).
Here clang is still assumed to be installed locally, since on Windows there are problems to start the clang in the artifact of Clang_jll.

@tshort
Copy link
Owner

tshort commented Feb 1, 2024

It'd be really nice to get tests working on Windows. For that, it seems like we need to get the interface to Clang_jll to work. @Thomas008, what are the "problems to start the clang" that you mention?

@tshort
Copy link
Owner

tshort commented Feb 1, 2024

What is the file test/stc_test.jl for? I don't see where that's used.

@Thomas008
Copy link
Contributor Author

When starting the clang of the artifact I get a Windows error:
The procedure entry point "_ZSt14_once_functor" could not be located in the
DLL C:\Julia-1.10.0\bin\libLLVM-15jl.dll
I have documented this as an issue in Clang_jll:
JuliaPackaging/Yggdrasil#8015

The file test/stc_test.jl was for my testing only, it is not essential. To me it was useful to test what is working, and what is not working.

@brenhinkeller
Copy link
Collaborator

Is the cmd /c strictly necessary in

run(`cmd /c clang -Wno-override-module $wrapper_path $ir_path -o $exec_path`)

?
It looks like that starts a new CMD shell, but presumably run is already starting one?

Copy link
Collaborator

@brenhinkeller brenhinkeller left a comment

Choose a reason for hiding this comment

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

We should probably remove the test files that are unused by the testsystem -- if they're not integrated then perhaps best just to keep them as separate test scripts outside the repo

@@ -98,13 +98,13 @@ shell> ./hello
Hello, world!
```
"""
function compile_executable(f::Function, types=(), path::String="./", name=fix_name(f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed this to pwd() which should be system-agnostic, whereas "./" was unix-only

# Clean up
run(`rm $wrapper_path`)
rm(wrapper_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This (Julia's built-in rm) should be system-agnostic and avoid having to special-case Windows

filename = name,
demangle = true,
cflags = ``,
target::StaticTarget=StaticTarget(),
llvm_to_clang = Sys.iswindows(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've added a kwarg to enable this alternative compilation pipeline. This will then be on by default on windows, and also allows us to test the alternate pipeline more generally in our non-windows CI (or if anyone else ever wants it, for any reason)

@brenhinkeller
Copy link
Collaborator

Ok, I think this is mergeable in principle now. We don't have Windows CI yet, but we do have tests on the alternative just-give-llvm-to-clang compilation pipeline in general since that's been split out as a kwarg that is true by default on Windows

@brenhinkeller
Copy link
Collaborator

Should close #137

@brenhinkeller brenhinkeller merged commit e672f35 into tshort:master Mar 4, 2024
13 of 16 checks passed
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.

3 participants