-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add a new Image class which allows using images in GLSL #388
Conversation
General comments (from @axsaucedo) Renaming of classes Tensor->Vec, Image->Mat: height, weight -> x,y. Memory object: Manager.hpp: Tests: Operations: On the point for "A more recent issue asks for 8-bit data support": kompute/test/TestOpShadersFromStringAndFile.cpp Lines 12 to 59 in a155585
Is there something you meant further to just adding uint8_t to the supported datatypes? @SimLeek, on this point "Casting everything to float through python and back is basically a hack": I'm trying to understand what you mean, from the current impl. it would be using numpy dtypes, do you mean when converting to array? Also on this point "Oh, also, I kinda want custom data types as a different issue, like sending structs to buffers.": Does the link above answer the question or you meant something else? |
TODO:
Answered:
There is now.
Fixed.
The reason is that now the Sequence->eval uses Memory instead of Tensor and it looks like the compiler doesn't know how to convert std::vector<std::shared_ptr> to std::vector<std::shared_ptr>. For example, this snippet from TestASyncOperations.cpp does not work: std::vector<std::shared_ptr<kp::Tensor>> inputsSyncB;
...
sq->eval<kp::OpTensorSyncDevice>(inputsSyncB); because:
Because std::shared_ptr is defined there.
Only required for tests.
Done in the latest version. Now there is just OpSyncLocal and OpSyncDevice.
Not required. Fixed. Deferred:
|
Test custom dtype for vectors vs image?I'm not completely sure what this meant. Custom types don't make sense for images as you can only create them with the formats Vulkan/the HW supports. You can't have images that contain structs etc for example. |
Good point, it seems indeed the Image type would not support the dtype, may be worth covering this as a compile-type error if anyone tries |
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.
Thank you for the PR @robquill - I've done an initial pass and added a few comments, let me know if any thoughts or questions
A few more points to add to the actions:
- Adding further python tests to identify issues/limits with tensor/image interactions
- Explore/discuss exposing further functionality through top level parent class
- Implement or extend 1-2 of the more advanced examples with image class to further battle test the limitations (can be done as a separate PR)
- Exploring how the height,width->x,y could also be exposed top level class (Tensor class could also have the same as default - i.e. x,y->x,1)
I've also been thinking about the design and I do feel like exposing more functionality to the top level class makes sense so it can be used without losing any functionality (+ simplifying API), so it's possible to have full abstraction on the underlying objects. Thinking that this could also allow for the python interface to enable for similar top level abstraction with this base class.
I'm not sure how to make this a compile-time error because the dtype is passed as a variable. |
Apologies, it seems I didn't share full context. In the current implementation there's nothing that would have to be done. However following the suggestion here of merging the imageType and tensorType into a top level enum it would be required as it would by default have an However indeed it would only be necessary given the suggestion above - although hasn't yet been discussed. Let me know if this is not clear and I can expand. |
OK, let me see how much I can refactor into the Memory class then we can see what to do about this. I don't completely understand the purpose of merging Image and Tensor and having an enum to differentiate. You would just end up replacing dynamic_cast with a test of the enum, which I imagine is what the compiler ends up doing internally to do the dynamic_cast. |
Just to clarify, it wouldn't be merging the entire image and tensor class, but only the imageDataType and tensorDataType into a single enum In regards to the dynamic_cast, I have to say that overall I don't have big issues with this implementation, as I liked how it was used - however seeing the need for a dynamic cast makes me think that having an explicit |
OK, the latest code has imageDataType and tensorDataType replaced with a single enum Memory::DataType. As part of that I added support for Tensors of u/int8/16_t as these formats were already supported for images. |
@axsaucedo , most of the major refactoring/consolidation is now done if you want to give this another review. There are some small bits like x,y still to do but it should be in a lot better shape now. So far I don't think we need an enum for tensor/image but see what you think and let me know what you want to do (if anything) with custom dtypes for images. |
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 looks good, overall quite clean as well without major modifications to the API, as it seems most examples work as is (only with the OpTensorX -> OpX change). I added a few comments but these are minor. We should be able to move to finalising the last few admin points (docs, codecov, etc) and wrapping up in order to merge. Thank you for the contribution!
Specifically for the custom dtypes for images, we can add a static_assert to ensure the imageT class implementation is restricted to the supported types, but let me know if other ideas.
src/include/kompute/Memory.hpp
Outdated
* @param commandBuffer Vulkan Command Buffer to record the commands into | ||
* @param copyFromMemory Memory to copy the data from | ||
*/ | ||
void |
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.
Indentation
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.
Fixed
Code coverage generally looks good (see attached). Most of the lines that aren't reached are because they are error cases that are difficult/impossible to hit. See the attached report. |
@axsaucedo , does this updated diagram look OK to you? https://drive.google.com/file/d/1iKMY9YPKT97WgjYSYQ8LNHmRyXPwwx-H/view?usp=sharing |
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.
LGTM! We can update documentation as a separate PR + with respective naming convention - DCO check is missing so we can merge.
921577a
to
aad7bed
Compare
@axsaucedo , I've made a bit of a mess of the rebase by trying to fix the DCO. The end state should be unchanged but I'm thinking of just squashing all the commits into one as now the middle is a bit of a mess. Any thoughts/objections before I go ahead and do it? |
@robquill no objections, we would squash on merge otherwise |
Create a Memory base-class which Image and Tensor inherit from. Operations and Sequences now use the Memory base-class instead of the Tensor class. OpTensorCopy has been renamed to OpCopy and supports copying between Tensors and Tensors, Tensors and Images, Images and Tensors and Images and Images. OpTensorSyncDevice/Local has been renamed to OpSyncDevice/Local and supports synchronising both Tensors and Images. Signed-off-by: Robert Quill <robert.quill@imgtec.com>
bccb9b6
to
36efab3
Compare
Signed-off-by: Robert Quill <robert.quill@imgtec.com>
Create a Memory base-class which Image and Tensor inherit from.
Operations and Sequences now use the Memory base-class instead of the Tensor class.
OpTensorCopy has been renamed to OpCopy and supports copying between Tensors and Tensors, Tensors and Images, Images and Tensors and Images and Images.
OpTensorSyncDevice/Local has been renamed to OpSyncDevice/Local and supports synchronising both Tensors and Images.