-
Notifications
You must be signed in to change notification settings - Fork 155
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
Update Various C++ Documentation Examples to Current Interface #398
Conversation
Updated so the example compiles in a test project. With the addition of an Image class various things were reorganized, including algorithm using kp::Memory instead of just kp::Tensor and kp::OpTensorSync* operations renamed to kp::OpSync*. Signed-off-by: Keith Horrocks <KeithJH@users.noreply.github.com>
With the addition of an Image class various things were reorganized, including algorithm using kp::Memory instead of just kp::Tensor and kp::OpTensorSync* operations renamed to kp::OpSync*. Tested building with KOMPUTE_OPT_ENABLE_BENCHMARK="ON" and verifying resulting binary runs. Signed-off-by: Keith Horrocks <KeithJH@users.noreply.github.com>
More renames from kp::OpTensorSync* to kp::OpSync* Example now compiles in a test project Signed-off-by: Keith Horrocks <KeithJH@users.noreply.github.com>
Various corrections, including renaming kp::OpTensorSync* operations to kp::OpSync*. Example now compiles in a test project. Signed-off-by: Keith Horrocks <KeithJH@users.noreply.github.com>
Various corrections, including renaming kp::OpTensorSync* operations to kp::OpSync*. Example now compiles when pieced together in a test project. Signed-off-by: Keith Horrocks <KeithJH@users.noreply.github.com>
Various corrections, including renaming kp::OpTensorSync* operations to kp::OpSync*. Example now compiles when pieced together in a test project. Signed-off-by: Keith Horrocks <KeithJH@users.noreply.github.com>
|
||
// Run the second parallel operation in the `queueTwo` sequence | ||
sqTwo->evalAsync<kp::OpAlgoDispatch>(algo); | ||
sqTwo->evalAsync<kp::OpAlgoDispatch>(algoTwo); |
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.
Is there a better way to signify running the same shader but with a different tensor bound for this example other than creating another Algorithm
? The code before my change just ran both sequences updating tensorA
and tensorB
was never updated. The below print appears to expect both updated, which makes sense for a more useful example.
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'm not sure I follow, could you provide an example of what you mean?
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.
@axsaucedo Anything that more closely resembles what the code was previously trying to do, with reuse of algo
instead of creating two separate algorithms. I've not looked into the backend, but this could be to avoid any overhead around spirv
being duplicated (like making sure it's loaded/ready on device).
I'm going to guess that since there wasn't an immediate suggestion to change here that there is no concern in having the separate algorithms and we should continue as-is.
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.
Perhaps a poor example as it doesn't make sense with how everything is laid out, but I could imagine a world where you would see something similar to:
sqOne->evalAsync<kp::OpAlgoDispatch>(algo, tensorA);
sqTwo->evalAsync<kp::OpAlgoDispatch>(algo, tensorB);
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.
Ah I see what you mean now - yes this would actually be much desirable. When designing kompute this was initial aim as well, unfortunately due to the design of the underlying vulkan architecture it's not possible (namely due to the dependency between the descriptor sets, the algorithm and the tensors) which make it such that initialisation is coupled. This is something that I hope at some point is addressed in the design of Vulkan, but at least in the medium term this doesn't seem to be planned. Hope this provides further context.
There are more potential changes, but this PR covers the ones I felt comfortable validating with my current development environment. Presumably changes are also necessary for the below. I can create issues for these to be handled separately. Python Examples:
C++ Reference Documentation:
Godot code:
|
Amazing, thank you very much for the contribution, this is indeed well needed - much appreciated. |
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
Updated various C++ documentation examples and TestBenchmark to code that will compile. This largely is updating references of
kp::OpTensorSync{Device,Local}
tokp::OpSync{Device,Local}
and updating algorithms to usekp::Memory
instead ofkp::Tensor
as these changes were introduced in #388.Examples from documentation were tested by copying into a local C++ project setup to use Kompute and compile without any modification to the example code.
TestBenchmark was tested by compiling Kompute with
KOMPUTE_OPT_ENABLE_BENCHMARK="ON"
and ensuring the produced binary runs.