- Notifications
You must be signed in to change notification settings - Fork193
Add dev branch to CI#81
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a809765 to9cfd436Compare6976369 to2bc52bfCompare| Tensor tensor =createTensor(ctx, shape, ki8); | ||
| wgpuQueueWriteBuffer(ctx.queue, tensor.data.buffer,0, packed.data(), | ||
| tensor.data.size); | ||
| return tensor; |
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.
ki8 is packed into uint32_t while the shape is the same.
It caused out of bounds access and segmentation faults when copying data.
| // cbData->buffer needs to be freed, but calling it here will cause a segmentation fault. | ||
| // wgpuBufferRelease(cbData->buffer); |
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 don't know the appropriate time to release it, but if we run it here, a segmentation fault will occur.
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.
Yes, hard to manage, got stuck on this some too across platforms. Maybe we can return a pointer to the buffer and require the frontend consumer to release it?
cmake/dawn.cmake Outdated
| if(APPLE) | ||
| set(ABSEIL_COPTS_FILE"${DAWN_DIR}/third_party/abseil-cpp/absl/copts/GENERATED_AbseilCopts.cmake") | ||
| if(EXISTS"${ABSEIL_COPTS_FILE}") | ||
| file(READ"${ABSEIL_COPTS_FILE}" COPTS_CONTENT) | ||
| string(REGEX REPLACE"-msse4\\.1""" COPTS_CONTENT"${COPTS_CONTENT}") | ||
| file(WRITE"${ABSEIL_COPTS_FILE}""${COPTS_CONTENT}") | ||
| endif() | ||
| endif() |
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.
There are many differences due to the line ending code being changed from DOS to Unix, but this is the only part that needs to be corrected. There is no msse4.1 flag on MacOS, so we need to remove it.
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.
Thanks! I'm not familiar / rarely use the cmake builds.@MichealReed if you have a chance maybe could see if this these changes make sense (could be after we merge to dev).
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.
@junjihashimoto maybe we can just merge the changes fromhttps://github.com/Practicalxr/minigpu/blob/master/minigpu_ffi/src/cmake/dawn.cmake? This successfully builds Dawn for mobile platforms now too. I think the core of getting it to build on Mac/iOS was disabling glfw and tint test/cmd/examples.
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.
@MichealReed I tried using the dawn.cmake with gpu.cpp.
I get the following error:
gpu.cpp/external/dawn/src/dawn/native/metal/PhysicalDeviceMTL.mm:140:50: error: use of undeclared identifier 'kIOMainPortDefault' 140 | AcquireIORef(IOServiceGetMatchingService(kIOMainPortDefault, matchingDict.Detach()));This is due to differences in OS versions. Setting kIOMainPortDefault to 0 allows you to proceed.
| STDLIB := -stdlib=libc++ | ||
| endif | ||
| FLAGS=-std=c++17$(STDLIB) -I$(GPUCPP) -I$(GPUCPP)/third_party/headers -L$(GPUCPP)/third_party/lib run.cpp -ldl -lwebgpu_dawn | ||
| FLAGS=-std=c++17$(STDLIB) -I$(GPUCPP) -I$(GPUCPP)/third_party/headers -I$(GPUCPP)/third_party/headers/webgpu -L$(GPUCPP)/third_party/lib run.cpp -ldl -lwebgpu_dawn -Wl,-rpath,$(GPUCPP)/third_party/lib |
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 adds a search path to the program so that we don't need to set LD_LIBRARY_PATH.
examples/hello_world/run.cpp Outdated
| for (int i =0; i < N; ++i) { | ||
| inputArr[i] =static_cast<float>(i) /10.0;// dummy input data | ||
| } | ||
| std::cout << Shape{N} << std::endl; |
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.
minor nit, but generally try to avoid iostream (mostly use cstdio)
gpu.hpp Outdated
| } | ||
| }; | ||
| inline std::ostream&operator<<(std::ostream& os,const Shape& shape) |
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.
(same as above re avoiding iostream)
Thanks very much. Any objections to removing the iostream bits? Besides that, can go ahead and merge to dev. Would be helpful if@MichealReed or others using the cmake builds could chime in on the cmake implementation as I'm not that familiar with the implementation / usage. |
@austinvhuang |
22692e2 intoAnswerDotAI:devUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The dev branch has compilation errors due to an incorrect INCLUDE header.
This PR adds the dev branch to CI and fixes the error.