Add alignment support to contiguous resource #181
Merged
Conversation
This file contains 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
Previously, the contiguous memory resource ignored its allocation parameter, which lead to errors - in particular issue acts-project#180. This commit reworks the internal allocation strategy to use std::align, ensuring that memory is properly aligned. This also breaks (in a valid way) some of the tests, which are adapted to support the new behaviour. Closes acts-project#180.
krasznaa
approved these changes
Jun 27, 2022
There was a problem hiding this comment.
I think std::align is the way to go. Just 2 comments, which don't necessarily need to happen in this PR. But they should happen somewhere.
- Every other downstream memory resource needs to be taught to respect the alignment requests. Most importantly the caching allocators. (@Yhatoh, how do you feel about teaching
vecmem::arenam_memory_resourceto deal with alignments correctly as well? 😉) - We should add unit tests that exercise the alignment properly. Including some "device tests" that would check if memory created through "weird allocations" could be correctly handled by some dummy kernels.
This was referenced Jun 28, 2022
|
Encoded the requirements into new issues, so we can merge this in as far as I'm concerned. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, the contiguous memory resource ignored its allocation parameter, which lead to errors - in particular issue #180. This commit reworks the internal allocation strategy to use
std::align, ensuring that memory is properly aligned.This also breaks (in a valid way) some of the tests, which are adapted to support the new behaviour.
Closes #180.