Skip to content

Build refactor + Windows #16

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

Merged
merged 25 commits into from
Jul 19, 2024

Conversation

MichealReed
Copy link
Contributor

@MichealReed MichealReed commented Jul 18, 2024

Happy to receive feedback before merge on this build refactor and windows support addition.

  • Adds windows support to makefiles
  • Cross-platform support and linking via cmake
  • Python script checks local before downloading
  • Build library when make executed from root.
  • Migrate to single Makefile from the examples folder.
  • Resolve conflicts
  • Document Windows and WSL dependencies.
  • Upload all the build objects!
  • Test all the variants!

libdawn_x64_Release.zip

Release build works on Windows. Only tested hello_world, will test WSL and other variants before merge.

Requesting testers for varying linux distros and MacOS.

@MichealReed MichealReed changed the title Build refactor windows Build refactor Windows Jul 18, 2024
@austinvhuang
Copy link
Contributor

Thanks for this effort! Appreciate the diligence on this, it's always an uphill battle keeping C++ build complexity under control.

Can you think of ways to simplifying or consolidating duplicate aspects of build code between the examples? On one hand, I like self-contained examples and am fine with some duplication. However the amount of supporting code per example is increasing so it might be worth it. What do you think?

@MichealReed
Copy link
Contributor Author

MichealReed commented Jul 18, 2024

Thanks for this effort! Appreciate the diligence on this, it's always an uphill battle keeping C++ build complexity under control.

Can you think of ways to simplifying or consolidating duplicate aspects of build code between the examples? On one hand, I like self-contained examples and am fine with some duplication. However the amount of supporting code per example is increasing so it might be worth it. What do you think?

That commit simplifies things a bit on the cmake side, not sure what to do with the makefiles. The only thing that needs to change file to file is TARGET.

CMakeLists.txt Outdated
add_library(gpu SHARED ${SRC_LIB})
set_target_properties(gpu PROPERTIES LINKER_LANGUAGE CXX)
# Add subdirectories for examples
add_subdirectory(examples/hello_world)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make a CMakeLists.txt in the examples folder and include the examples in that instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, keep the root CMakeLists to building the library, I like that. Will handle this with the Makefile changes from Austin.

@austinvhuang
Copy link
Contributor

That commit simplifies things a bit on the cmake side, not sure what to do with the makefiles. The only thing that needs to change file to file is TARGET.

The option I lean toward might be to have one Makefile in the examples directory. Then we can also compress the repeated patterns using a default rule like this:
https://github.com/karpathy/llm.c/blob/85d17f4aa24beaa7cac688f37f7803ccc0dacb12/dev/cuda/Makefile#L28

Another option is to keep the separate Makefiles and use include of a shared Makefile which has the common targets (like check-* and possibly the default rule).

If you want to leave the Makefile simplification out of scope for this PR that's fine too, there's already quite a lot here.

@MichealReed
Copy link
Contributor Author

That commit simplifies things a bit on the cmake side, not sure what to do with the makefiles. The only thing that needs to change file to file is TARGET.

The option I lean toward might be to have one Makefile in the examples directory. Then we can also compress the repeated patterns using a default rule like this: https://github.com/karpathy/llm.c/blob/85d17f4aa24beaa7cac688f37f7803ccc0dacb12/dev/cuda/Makefile#L28

Another option is to keep the separate Makefiles and use include of a shared Makefile which has the common targets (like check-* and possibly the default rule).

If you want to leave the Makefile simplification out of scope for this PR that's fine too, there's already quite a lot here.

One Makefile in the examples folder seems like a great idea. I do not think it will be too difficult, I will experiment with some structures for it soon. Good synergy for this PR.

@MichealReed
Copy link
Contributor Author

We're building the library but nothing is exported yet, will scope exports for FFI usage to a new PR. Opened #17

@MichealReed MichealReed changed the title Build refactor Windows Build refactor + Windows Jul 18, 2024
@MichealReed
Copy link
Contributor Author

Had to apt-get the following for WSL, not sure if it's worth adding to the readme.

libx11-xcb-dev libx11-dev libxi-dev libxcursor-dev libxinerama-dev libxrandr-dev libx11-6 libc++-dev libvulkan1 mesa-vulkan-drivers vulkan-tools clang

@austinvhuang
Copy link
Contributor

Had to apt-get the following for WSL, not sure if it's worth adding to the readme.

libx11-xcb-dev libx11-dev libxi-dev libxcursor-dev libxinerama-dev libxrandr-dev libx11-6 libc++-dev libvulkan1 mesa-vulkan-drivers vulkan-tools clang

Could add in a bullet in the "Quick Start: Building and Running" section after "Only on Linux systems ..." something like:

"- Only on Windows WSL - Install vulkan and libx11 ..."

endif()
message(STATUS "Using WebGPU distribution tag: ${WEBGPU_TAG}")

if (WEBGPU_TAG STREQUAL "dawn")
Copy link
Contributor

Choose a reason for hiding this comment

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

At the time I think I ran into some issues trying to pin to a specific commit in the dawn branch, probably mostly due to my cmake knowledge limitations (i could get it to pin to a commit hash on main, but had issues with commit hashes on the branch).

If you know of an easy way to do it, that might be preferable for reproducibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give me a couple of tags to test with? Was this working before?

@@ -7,6 +7,10 @@
#include "gpu.h" // createContext, createTensor, createKernel, dispatchKernel,
// wait, resetCommandBuffer, toCPU

#ifndef M_PI
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC this is because _USE_MATH_DEFINES is not set in MSVC builds -- maybe a better approach is to define that value as a preprocessor directive in CMakeLists and Makefile.

@austinvhuang
Copy link
Contributor

Some of the build targets need updating: https://github.com/AnswerDotAI/gpu.cpp/actions/runs/10002790401/job/27650314641?pr=16
but i'll go ahead and merge this first and push a patch this morning.

@austinvhuang austinvhuang merged commit 1f5c75f into AnswerDotAI:main Jul 19, 2024
1 check failed
austinvhuang added a commit that referenced this pull request Jul 19, 2024
…ept for wsl2) native branching code, provide feedback on file load failure in shadertui, minor examples build cleanups
austinvhuang added a commit that referenced this pull request Jul 19, 2024
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