Add dev branch to CI #81
Conversation
junjihashimoto
force-pushed
the
fix/dev-ci
branch
from
181b592 to
c5f7a00
Compare
junjihashimoto
force-pushed
the
fix/dev-ci
branch
2 times, most recently
from
a809765 to
9cfd436
Compare
junjihashimoto
force-pushed
the
fix/dev-ci
branch
6 times, most recently
from
6976369 to
2bc52bf
Compare
junjihashimoto
force-pushed
the
fix/dev-ci
branch
from
2bc52bf to
bcb81e1
Compare
| 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.
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.
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.
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.
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.
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.
@junjihashimoto maybe we can just merge the changes from https://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.
@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.
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.
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.
(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 |
3 participants
The dev branch has compilation errors due to an incorrect INCLUDE header.
This PR adds the dev branch to CI and fixes the error.