Remove explicit checks against kp::Memory::type()#455
Merged
axsaucedo merged 2 commits intoKomputeProject:masterfrom Apr 13, 2026
Merged
Remove explicit checks against kp::Memory::type()#455axsaucedo merged 2 commits intoKomputeProject:masterfrom
axsaucedo merged 2 commits intoKomputeProject:masterfrom
Conversation
ahem
force-pushed
the
fix-algorithm-descriptor-type-check
branch
from
27697df to
401d1b6
Compare
The function that was prevously used, `mem->type()`, is not explicitly the same as the descriptor type, so using `mem->getDescriptorType()` is more accurate. It also makes it easier to create new image-like subclasses of `kp::Memory` because as long as they specify the correct descriptor type, the correct pools will be created. Signed-off-by: Anders Hellerup Madsen <anders@hellerup-madsen.dk>
ahem
force-pushed
the
fix-algorithm-descriptor-type-check
branch
from
401d1b6 to
b888179
Compare
The if statement used to assume that if `mem->type() == memory::Type::eImage` then it was safe to assume that the memory object is an instance of `kp::Image`. However, any subclass of `kp::Memory` could implement the virtual `type()` method to return `memory::Type::eImage`, so this is an invalid assumption. This commit fixes the issue by only casting if the memory object actually is an instance of `kp::Memory` Signed-off-by: Anders Hellerup Madsen <anders@hellerup-madsen.dk>
ahem
force-pushed
the
fix-algorithm-descriptor-type-check
branch
from
dbdfab5 to
94a1449
Compare
ahem
changed the title
Contributor
Author
|
@axsaucedo Sorry for updating the PR a few times. It is ready for review now. |
Member
|
Great catch - thanks for the contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is a few places in the codebase where assumptions are being made based on what a memory objects virtual
kp::Memory::type()method returns. This makes it hard to create new subclasses because there is an implicit assumption that iftype()returnsmemory::Type::eImagethen it iskp::Imageand will behave like that.This PR fixes two of those issues:
Algorithm.cppto callmem->getDescriptorType()instead ofmem->type()when allocating descriptor pools. With the current subclasses ofkp::Memorythis does not make a difference, but the explicit checks against memory type makes it hard to create new subclasses.OpAlgoDispatch.cppto actually check if the memory object is safe to cast tokp::Imageinstead of assuming that ifmem->type()returnsmemory::Type::eImagethis casting is safe.With these changes it becomes safe to create new subclasses of
kp::Memorythat wraps images and uses image descriptors internally, without forcing those to subclasskp::Image(which isn't virtual and can't be subclassed anyway).Those new subclasses will not really work with the strong-typed copy methods but they can implement their own copy methods for that if they feel like it.