KomputeProject / kompute

General purpose GPU compute framework built on Vulkan to support 1000s of cross vendor graphics cards (AMD, Qualcomm, NVIDIA & friends). Blazing fast, mobile-enabled, asynchronous and optimized for advanced GPU data processing usecases. Backed by the Linux Foundation.
http://kompute.cc/
Apache License 2.0
1.97k stars 151 forks source link

Enhance Tensor Flexibility with Structs #327

Open BrianPetkovsek opened 1 year ago

BrianPetkovsek commented 1 year ago

replaces kp::Tensor::TensorDataTypes with std::type_info. This allows you to use any struct as a tensor rather than being limited by base datatypes (int, uint, float, etc..).

The easiest way I found to use structs in shaders is to use the "GL_GOOGLE_include_directive" extension to include a hpp file that holds the struct. this way you can use the struct in your shader code and c++ code. (see new example)

axsaucedo commented 1 year ago

@BrianPetkovsek thank you for the contribution, this seems like a reasonable addition - also I wasn't aware of the GL_GOOGLE_include_directive, quite interesting. It seems relatively analogous to the way the Constants are impleemnted as these also have flexibility to be other structures.

In regards to performance would this have any distinction?

As a heads up there seems to still have build issues in the python package - the rest of the examples should also be tested to ensure they work correctly as although a small design change we just need to ensure the rest works - if you can validate the basic examples I can validate the heavier ones like the android / godot.

BrianPetkovsek commented 1 year ago

Hi @axsaucedo,

I’ve decided to change my approach and use an abstract class called ABCTypeContainer instead of typeid and std::type_info. This allows for the implementation of both a C++ and Python version, which I have already included.

I’ve made some significant changes to main.cpp for the pybind11 bindings, but I’m not entirely sure how push_consts, specconstsvec, and pushconstsvec are implemented. If they’re just buffers, it might be easier to make them a std::vector\<byte>.

The C++ type container could use some cleanup. Currently, it uses a counter system with the struct IdCounter. If there’s a way to consolidate this without using an external struct, it should be done.

C++ type container works by The classId() method in the C++ type container returns a unique identifier for each unique template type instantiation of the TypeContainer template. This is achieved by using a static variable id that is initialized with the value of counter from the IdCounter struct and then incremented, ensuring that each instantiation has a unique id.

The PyTypeContainer works by comparing the dtypes of numpy arrays. A dtype can act as either a structure or just a datatype, allowing for struct-like capabilities (see https://numpy.org/doc/stable/user/basics.rec.html#structured-arrays).

This also resolves #99 as you can put multi-dimensional arrays in structs

BrianPetkovsek commented 1 year ago

Awaiting review/comments.

BrianPetkovsek commented 1 year ago

fixed the issues with the tests.

I created a new class called Buffer. This class is a basic buffer class that holds Buffer data like pointer location, size, length, and end. positions. The class gets compiled away at compile time.

I have also updated the classes that uses push/spec constants to allow for Buffer without affecting the external api, (you can still use vectors as push/spec).

BrianPetkovsek commented 1 year ago

Awaiting review/comments.

BrianPetkovsek commented 1 year ago

Im compiling my build with MSVC , seems like gcc is more picky with compiling, fixed the build issues.

BrianPetkovsek commented 1 year ago

I'll downloads gcc and fix it

axsaucedo commented 1 year ago

Thank you for having a deeper look into this, I was able to get an initial review. I have a few followups:

I have also updated the classes that uses push/spec constants to allow for Buffer without affecting the external api, (you can still use vectors as push/spec).

Would you be able to provide further context on the reasoning for having a Buffer class? Is this to simplify the way that the python wrapper implements it? I feel this buffer adds extra complexity - I do agree that this is only used internally so may not be as bad, but I am wondering if this would be necessary or whether it's only for use in the python side?

On a side note, in regards to naming conventions we should not use conventions that exist in Vulkan to avoid confusion (ie Buffer =? vkBuffer)

In regards to the dataType it seems it's passed everywhere as a pointer, is there a reason why this is not just passed everywhere as reference? GIven its deterministic behaviour I would assume this coudl now be the case.