558 develop shared memory parallel backend for dense alp #163
Conversation
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
…backend (#129) Introduce essential functionalities of the parallel backend to support allocating a matrix in parallel and setting its elements to a single value.
* Replication factor in Distribution becomes static constexpr * Take into account replication factor in thread grid layout * Consider full thread grid coordinates (including rt) in containers and operations * Add missing comments and fix a mistake in another comment * Avoid calling distribution functions multiple times * Pass thread coordinates using ThreadCoords object * Compute number of threads within the distribution * Encapsulate thread coordinates within the local coordinates * Remove thread-grid related structures and getters since they are no longer needed * Explain the reason behind hard-coded rt value * Calculate block id inside distribution * Rename distribution to a more descriptive name
…hared-memory-parallel-backend-for-dense-alp-backup
…lop-shared-memory-parallel-backend-for-dense-alp
* Started drafting mxm using 2.5D algo * WIP implementation of shifting + compute * First complete draft * compiling mxm test on general matrices * shared mem (ge) mxm passing functional tests * Tmp debugging info + fix in shift computation * Refactoring includes * - Fixing mismatching new/delete bug - Enabling allocation/computation on sub-grid of threads - Added unit tests for non-cubic mxm * Fixed style and typos
djelovina
reviewed
Mar 10, 2023
Comment on lines
-514
to
+516
| if [ "${BACKEND:0:4}" != "alp_" ]; then | ||
| # Temporarily execute tests only for alp_reference backend | ||
| # until all backends start supporting all smoke tests. | ||
| if [ "${BACKEND}" != "alp_reference" ]; then |
Collaborator
There was a problem hiding this comment.
Should we enable omp tests?
Comment on lines
+53
to
+56
| /** | ||
| * \internal general mxm implementation that all mxm variants using | ||
| * structured matrices refer to. | ||
| */ |
Collaborator
There was a problem hiding this comment.
A more detailed explanation would be useful.
| > | ||
| RC mxm_generic( | ||
| alp::Matrix< OutputType, OutputStructure, | ||
| Density::Dense, OutputView, OutputImfR, OutputImfC, omp > &C, |
Collaborator
There was a problem hiding this comment.
Indent (here and in the following lines) is missing.
I think we follow this style:
alp::Matrix<
InputType1, InputStructure1,
Density::Dense, InputView1, InputImfR1, InputImfC1, omp
> &A,
Comment on lines
+134
to
+152
| // /** Type encapsulating the local block coordinate. */ | ||
| // struct LocalBlockCoord { | ||
|
|
||
| // const size_t tr; | ||
| // const size_t tc; | ||
| // const size_t rt; | ||
| // const size_t br; | ||
| // const size_t bc; | ||
|
|
||
| // LocalBlockCoord( | ||
| // const size_t tr, const size_t tc, | ||
| // const size_t rt, | ||
| // const size_t br, const size_t bc | ||
| // ) : | ||
| // tr( tr ), tc( tc ), | ||
| // rt( rt ), | ||
| // br( br ), bc( bc ) {} | ||
|
|
||
| // }; |
Collaborator
There was a problem hiding this comment.
This is not needed any more. There are several commented-out blocks in this file. Perhaps some of them should be removed?
| RC local_rc = SUCCESS; | ||
|
|
||
| // Broadcast A and B to all c-dimensional layers | ||
| if( local_rc == SUCCESS && da.isActiveThread( th_ijk_a ) && th_ijk_a.rt > 0 ) { |
Collaborator
There was a problem hiding this comment.
the check local_rc == SUCCESS is always true
Comment on lines
+186
to
+195
| // LocalBlockCoord mapBlockGlobalToLocal( const GlobalBlockCoord &g ) const { | ||
| // (void) g; | ||
| // return LocalBlockCoord( 0, 0, 0, 0, 0 ); | ||
| // } | ||
|
|
||
| // GlobalBlockCoord mapBlockLocalToGlobal( const LocalBlockCoord &l ) const { | ||
| // const size_t block_id_r = l.br * Tr + l.tr; | ||
| // const size_t block_id_c = l.bc * Tc + l.tc; | ||
| // return GlobalBlockCoord( block_id_r, block_id_c );// Temporary | ||
| // } |
Collaborator
There was a problem hiding this comment.
Old comments, remove them?
|
|
||
| /** | ||
| * AMF for parallel shared memory backend. | ||
| * |
Collaborator
There was a problem hiding this comment.
Should we leave a space for generalization of this choice?
| imf_r( imf_r ), imf_c( imf_c ), | ||
| num_threads( num_threads ), | ||
| distribution( imf_r.n, imf_c.n, num_threads ) { | ||
| std::cout << "Entering AMF normal constructor\n"; |
Collaborator
There was a problem hiding this comment.
Should this message got to DEBUG?
| imf_c( std::move( amf.imf_c ) ), | ||
| num_threads( amf.num_threads ), | ||
| distribution( std::move( amf.distribution ) ) { | ||
| std::cout << "Entering OMP AMF move constructor\n"; |
Collaborator
There was a problem hiding this comment.
Same comment about message.
| * for R and/or C. | ||
| * | ||
| */ | ||
| storage_index_type getStorageIndex( const size_t i, const size_t j, const size_t s, const size_t P ) const { |
Collaborator
There was a problem hiding this comment.
Is const size_t P needed here?
Member
|
Slot for 0.9 - but @djelovina , is this one still current or should we focus on the one you more recently rebased? |
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.
This PR merges current omp reference backend into 303. Future extensions on either sequential or parallel backends would use feature branches from 303 only.