Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor Memory Access Iterators #75

Merged
merged 5 commits into from
Feb 15, 2022
Merged

Refactor Memory Access Iterators #75

merged 5 commits into from
Feb 15, 2022

Conversation

SimeonEhrig
Copy link
Member

  • Rename Iterator to Memory Access Strategy
  • remove data member from iterator because:
  • not necessary for functionality
  • causes a bug on CPU backends with fancy iterators
  • create unnecessary temporary objects
  • harder to read
  • harder to optimize for the compiler

@SimeonEhrig
Rename Iterator to Memory Access Strategy
f905d91 
- rename was necessary to avoid confusions with fancy iterator
- rename and flat namespace of the mem access strategy
- move related files to new folder
@SimeonEhrig
Remove data member from the memory access strategy
dbc5d3a 
- now, the memory access strategy returns the index instead the actual data element
@bussmann
Copy link

I wonder if Vikunja memory access could profit from LLAMA...

@SimeonEhrig
Copy link
Member Author

I wonder if Vikunja memory access could profit from LLAMA...

For this PR, it is not interesting, because the current implementation of the memory access strategies requires 1D memory without padding. But in general, yes. I want to support also multidimensional memory #61 . I think std::mdspan is a good idea for the memory access, like @bernhardmgruber already suggested for llama and alpaka.

@SimeonEhrig
rename iterator test to MemAccessStrategy test
5b3f091 
- rename also folder hierarchy
@SimeonEhrig
Copy link
Member Author

After a discussion with @bernhardmgruber we find out, that std::mdspan does not support padding, when the padding is not multiple of the size of a single element. Therefore I will use alpaka::accessors, which supports this.

As side not, std::mdspan supports also other nice features like slicing. But I don't need this feature for vikunja.

@SimeonEhrig
Rename MemAccessStrategy.cpp to BaseStrategy.cpp
44bd5db 
- clean up of the test
- prepare for addition iterator tests
- add cmake helper function for tests
- add memory allocation function for tests
@SimeonEhrig SimeonEhrig marked this pull request as ready for review February 8, 2022 13:05
@SimeonEhrig
Copy link
Member Author

The RP is ready for merging. At the moment, some tests and documentation are missing. This will follow in another PR. This PR needs to be merged, because it is a show stopper for PR #62, #68, #71, #72.

j-stephan
j-stephan requested changes Feb 9, 2022
View reviewed changes
Copy link
Member

@j-stephan j-stephan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions:

include/vikunja/access/BaseStrategy.hpp Outdated Show resolved Hide resolved
include/vikunja/access/PolicyBasedBlockStrategy.hpp Outdated Show resolved Hide resolved
include/vikunja/access/PolicyBasedBlockStrategy.hpp Outdated Show resolved Hide resolved
include/vikunja/access/PolicyBasedBlockStrategy.hpp Outdated Show resolved Hide resolved
include/vikunja/access/PolicyBasedBlockStrategy.hpp Outdated Show resolved Hide resolved
include/vikunja/access/PolicyBasedBlockStrategy.hpp Outdated Show resolved Hide resolved
test/include/vikunja/test/AlpakaSetup.hpp Outdated Show resolved Hide resolved
@SimeonEhrig SimeonEhrig force-pushed the refactorMemAccessStrategy branch from b86b6fc to f3aaa68 Compare February 15, 2022 12:49
@SimeonEhrig SimeonEhrig merged commit 4baaeb4 into alpaka-group:master Feb 15, 2022
@SimeonEhrig SimeonEhrig added this to the release 0.2 milestone Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-stephan j-stephan j-stephan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
release 0.2
Development

Successfully merging this pull request may close these issues.
3 participants
@SimeonEhrig @bussmann @j-stephan