From 0f69539e48656097d90ace067c482f8c0c6d97e2 Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Wed, 30 Aug 2023 15:39:23 +0200 Subject: [PATCH 01/19] Unit-test for mixed domains matrices --- tests/unit/CMakeLists.txt | 4 + tests/unit/copyMixedDomainsMatrices.cpp | 212 ++++++++++++++++++++++++ tests/unit/unittests.sh | 7 + 3 files changed, 223 insertions(+) create mode 100644 tests/unit/copyMixedDomainsMatrices.cpp diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 983848ae7..5cd03cf77 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -256,6 +256,10 @@ add_grb_executables( copyVoidMatrices copyVoidMatrices.cpp BACKENDS reference reference_omp bsp1d hybrid hyperdags nonblocking ) +add_grb_executables( copyMixedDomainsMatrices copyMixedDomainsMatrices.cpp + BACKENDS reference reference_omp bsp1d hybrid hyperdags nonblocking +) + add_grb_executables( masked_muladd masked_muladd.cpp BACKENDS reference reference_omp bsp1d hybrid hyperdags nonblocking ) diff --git a/tests/unit/copyMixedDomainsMatrices.cpp b/tests/unit/copyMixedDomainsMatrices.cpp new file mode 100644 index 000000000..e87698f8f --- /dev/null +++ b/tests/unit/copyMixedDomainsMatrices.cpp @@ -0,0 +1,212 @@ +/* + * Copyright 2021 Huawei Technologies Co., Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include +#include +#include + +#include +#include + + +using namespace grb; + + +void grb_program( const size_t &n, grb::RC &rc ) { + Matrix< int > A_int( n, n, n ); + Matrix< void > A_void( n, n, n ); + + { // Build A_int and A_void: identity matrices + std::vector< size_t > A_coords( n ); + std::iota( A_coords.begin(), A_coords.end(), 0 ); + if( SUCCESS != + buildMatrixUnique( A_void, A_coords.data(), A_coords.data(), n, PARALLEL ) + ) { + std::cerr << "\t initialisation of A_void FAILED: rc is " + << grb::toString( rc ) << "\n"; + rc = FAILED; + return; + } + + std::vector< int > A_vals( n ); + std::fill( A_vals.begin(), A_vals.end(), 1 ); + if( SUCCESS != + buildMatrixUnique( A_int, A_coords.data(), A_coords.data(), A_vals.data(), n, PARALLEL ) + ) { + std::cerr << "\t initialisation of A_int FAILED: rc is " + << grb::toString( rc ) << "\n"; + rc = FAILED; + return; + } + } + + + + { // Try cast to ushort (should succeed) + Matrix< ushort > M_short( n, n, 0UL ); + rc = grb::set( M_short, A_int, Phase::RESIZE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_short, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + rc = grb::set( M_short, A_int, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_short, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + // Cast back to compare + Matrix< int > M_int( n, n, nnz( A_int ) ); + rc = grb::set( M_int, M_short, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_int, M_short ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + if( !std::equal( M_int.begin(), M_int.end(), A_int.begin() ) ) { + std::cerr << "\t FAILED: M_int != A_int\n"; + rc = FAILED; + return; + } + } + { // Try (fake-)cast to int (should succeed) + Matrix< int > M_int( n, n, 0UL ); + rc = grb::set( M_int, A_int, Phase::RESIZE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_int, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + rc = grb::set( M_int, A_int, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_int, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + if( !std::equal( M_int.begin(), M_int.end(), A_int.begin() ) ) { + std::cerr << "\t FAILED: M_int != A_int\n"; + rc = FAILED; + return; + } + } + { // Try cast to bool (should succeed) + Matrix< bool > M_bool( n, n, 0UL ); + rc = grb::set( M_bool, A_int, Phase::RESIZE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_bool, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + rc = grb::set( M_bool, A_int, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_bool, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + // Cast back to compare + Matrix< int > M_int( n, n, nnz( A_int ) ); + rc = grb::set( M_int, M_bool, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_int, M_short ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + if( !std::equal( M_int.begin(), M_int.end(), A_int.begin() ) ) { + std::cerr << "\t FAILED: M_int != A_int\n"; + rc = FAILED; + return; + } + } + { // Try cast to void (should succeed) + Matrix< void > M_void( n, n, 0UL ); + rc = grb::set( M_void, A_int, Phase::RESIZE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_void, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + rc = grb::set( M_void, A_int, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_void, A_int ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + // Cast back to compare + Matrix< int > M_int( n, n, nnz( A_int ) ); + rc = grb::set( M_int, M_void, 1, Phase::EXECUTE ); + if( rc != SUCCESS ) { + std::cerr << "\t set( M_int, M_void, 1 ) FAILED: rc is " + << grb::toString( rc ) << "\n"; + return; + } + if( !std::equal( M_int.begin(), M_int.end(), A_int.begin() ) ) { + std::cerr << "\t FAILED: M_int != A_int\n"; + rc = FAILED; + return; + } + } + + rc = SUCCESS; +} + +int main( int argc, char ** argv ) { + // defaults + bool printUsage = false; + size_t in = 100; + + // error checking + if( argc > 2 ) { + printUsage = true; + } + if( argc == 2 ) { + size_t read; + std::istringstream ss( argv[ 1 ] ); + if( ! ( ss >> read ) ) { + std::cerr << "Error parsing first argument\n"; + printUsage = true; + } else if( ! ss.eof() ) { + std::cerr << "Error parsing first argument\n"; + printUsage = true; + } else { + // all OK + in = read; + } + } + if( printUsage ) { + std::cerr << "Usage: " << argv[ 0 ] << " [n]\n"; + std::cerr << " -n (optional, default is 100): an integer test size.\n"; + return 1; + } + + std::cout << "This is functional test " << argv[ 0 ] << "\n"; + grb::Launcher< AUTOMATIC > launcher; + grb::RC out; + if( launcher.exec( &grb_program, in, out, true ) != SUCCESS ) { + std::cerr << "Launching test FAILED\n" << std::endl; + return 255; + } + if( out != SUCCESS ) { + std::cerr << std::flush; + std::cout << "Test FAILED (" << grb::toString( out ) << ")\n" << std::endl; + } else { + std::cout << "Test OK\n" << std::endl; + } + return 0; +} + diff --git a/tests/unit/unittests.sh b/tests/unit/unittests.sh index a7bf23b38..0b49d2421 100755 --- a/tests/unit/unittests.sh +++ b/tests/unit/unittests.sh @@ -670,6 +670,13 @@ for MODE in ${MODES}; do grep 'Test OK' ${TEST_OUT_DIR}/copyVoidMatrices_${MODE}_${BACKEND}_${P}_${T} || echo "Test FAILED" echo " " + echo ">>> [x] [ ] Testing copy-constructor of square matrices with mixed-domains" + echo " of size 1003." + $runner ${TEST_BIN_DIR}/copyMixedDomainsMatrices_${MODE}_${BACKEND} 1003 &> ${TEST_OUT_DIR}/copyMixedDomainsMatrices_${MODE}_${BACKEND}_${P}_${T} + head -1 ${TEST_OUT_DIR}/copyMixedDomainsMatrices_${MODE}_${BACKEND}_${P}_${T} + grep 'Test OK' ${TEST_OUT_DIR}/copyMixedDomainsMatrices_${MODE}_${BACKEND}_${P}_${T} || echo "Test FAILED" + echo " " + if [ "$BACKEND" = "bsp1d" ] || [ "$BACKEND" = "hybrid" ]; then echo "Additional standardised unit tests not yet supported for the ${BACKEND} backend." echo From 044e3171ad16e8a713ed30b7054bb9f5a8b1fd81 Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Wed, 30 Aug 2023 18:49:21 +0200 Subject: [PATCH 02/19] Remove redundancy + switch memcpy to std::copy_n - Removed redundancy in the Compressed_Storage::copyFrom(...) functions. The section in charge of copying the row_index and col_start arrays has been moved to an external local function. - Merged the cast/no-cast Compressed_Storage::copyFrom(...) functions using std::copy_n rather than a for-loop/memcpy switch --- include/graphblas/nonblocking/io.hpp | 3 +- .../reference/compressed_storage.hpp | 273 +++++++++--------- include/graphblas/reference/io.hpp | 43 +-- 3 files changed, 158 insertions(+), 161 deletions(-) diff --git a/include/graphblas/nonblocking/io.hpp b/include/graphblas/nonblocking/io.hpp index 95147f225..beaaf4df5 100644 --- a/include/graphblas/nonblocking/io.hpp +++ b/include/graphblas/nonblocking/io.hpp @@ -1134,7 +1134,8 @@ namespace grb { std::cout << "Called grb::set (matrix-to-matrix, nonblocking)" << std::endl; #endif // static checks - NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || + NO_CAST_ASSERT( + ( !(descr & descriptors::no_casting) || std::is_same< InputType, OutputType >::value ), "grb::set", "called with non-matching value types" ); diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 2bb7bb183..37883845f 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -30,6 +30,84 @@ namespace grb { namespace internal { + template< typename D, typename IND, typename SIZE > + class Compressed_Storage; + + namespace { + + /** + * Copies row_index and col_index from a + * given Compressed_Storage to another. + * + * Performs no safety checking. Performs no (re-)allocations. + * + * @param[out] output The container to copy the coordinates to. + * @param[in] input The container to copy the coordinates from. + * @param[in] nz The number of nonzeroes in the \a other container. + * @param[in] m The index dimension of the \a other container. + * @param[in] k The start position to copy from (inclusive). + * @param[in] end The end position to copy to (exclusive). + * + * The copy range is 2nz + m + 1, i.e., + * -# 0 <= start < 2nz + m + 1 + * -# 0 < end <= 2nz + m + 1 + * + * Concurrent calls to this function are allowed iff they consist of + * disjoint ranges \a start and \a end. The copy is guaranteed to be + * complete if the union of ranges spans 0 to 2nz + m + 1. + */ + template< + typename OutputType, typename OutputIND, typename OutputSIZE, + typename InputType, typename InputIND, typename InputSIZE + > + static inline void copyCoordinatesFrom( + Compressed_Storage< OutputType, OutputIND, OutputSIZE > &output, + const Compressed_Storage< InputType, InputIND, InputSIZE > &input, + const size_t nz, const size_t m, + size_t k, size_t end + ) { + static_assert( std::is_convertible< InputIND, OutputIND >::value, + "InputIND must be convertible to OutputIND" + ); + static_assert( std::is_convertible< InputSIZE, OutputSIZE >::value, + "InputSIZE must be convertible to OutputSIZE" + ); + + if( k < nz ) { + const size_t loop_end = std::min( nz, end ); + assert( k <= loop_end ); + std::copy_n( + input.row_index + k, + loop_end - k, + output.row_index + k + ); + k = 0; + } else { + assert( k >= nz ); + k -= nz; + } + if( end <= nz ) { + return; + } + end -= nz; + if( k < m + 1 ) { + const size_t loop_end = std::min( m + 1, end ); + assert( k <= loop_end ); + std::copy_n( + input.col_start + k, + loop_end - k, + output.col_start + k + ); + #ifndef NDEBUG + for( size_t chk = k; chk < loop_end - 1; ++chk ) { + assert( input.col_start[ chk ] <= input.col_start[ chk + 1 ] ); + assert( output.col_start[ chk ] <= output.col_start[ chk + 1 ] ); + } + #endif + } + } + } // namespace + /** * Basic functionality for a compressed storage format (CRS/CSR or CCS/CSC). * @@ -516,21 +594,17 @@ namespace grb { } /** - * Copies contents from a given Compressed_Storage. + * Copies coordinates from a given Compressed_Storage, then fills the + * values with the given identity. * * Performs no safety checking. Performs no (re-)allocations. * - * @tparam use_id If set to true, use \a id instead of values in - * \a other. - * * @param[in] other The container to copy from. * @param[in] nz The number of nonzeroes in the \a other container. * @param[in] m The index dimension of the \a other container. * @param[in] start The start position to copy from (inclusive). * @param[in] end The end position to copy to (exclusive). * @param[in] id A pointer to a value overriding those in \a other. - * Will only be used if and only if \a use_id is set - * true. * * The copy range is 2nz + m + 1, i.e., * -# 0 <= start < 2nz + m + 1 @@ -541,36 +615,27 @@ namespace grb { * complete if the union of ranges spans 0 to 2nz + m + 1. */ template< - bool use_id = false, - typename InputType, typename ValueType = char + bool useId = true, + typename InputType, typename InputIND, typename InputSIZE, + typename ValueType > void copyFrom( - const Compressed_Storage< InputType, IND, SIZE > &other, + const Compressed_Storage< InputType, InputIND, InputSIZE > &other, const size_t nz, const size_t m, const size_t start, size_t end, - const ValueType * __restrict__ id = nullptr + const ValueType * __restrict__ id, + const typename std::enable_if< useId, void >::type * = nullptr ) { #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom (cast) called with range " - << start << "--" << end; - if( use_id ) { - std::cout << ". The identity " << (*id) << " will be used.\n"; - } else { - std::cout << ". No identity will be used.\n"; - } + << start << "--" << end << ". The identity " << (*id) << " will be used.\n"; #endif assert( start <= end ); size_t k = start; if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - for( ; k < loop_end; ++k ) { - if( use_id ) { - values[ k ] = *id; - } else { - values[ k ] = other.getValue( k, *id ); - } - } + std::fill_n( values + k, loop_end - k, *id ); k = 0; } else { assert( k >= nz ); @@ -580,50 +645,45 @@ namespace grb { return; } end -= nz; - if( k < nz ) { - const size_t loop_end = std::min( nz, end ); - assert( k <= loop_end ); - (void) std::memcpy( - row_index + k, - other.row_index + k, - (loop_end - k) * sizeof( IND ) - ); - k = 0; - } else { - assert( k >= nz ); - k -= nz; - } - if( end <= nz ) { - return; - } - end -= nz; - if( k < m + 1 ) { - const size_t loop_end = std::min( m + 1, end ); - assert( k <= loop_end ); - (void) std::memcpy( - col_start + k, - other.col_start + k, - (loop_end - k) * sizeof( SIZE ) - ); - } + + copyCoordinatesFrom( *this, other, nz, m, k, end ); } - /** \internal Specialisation for no cast copy */ - template< bool use_id = false > + /** + * Copies contents from a given Compressed_Storage. + * + * Performs no safety checking. Performs no (re-)allocations. + * + * @param[in] other The container to copy from. + * @param[in] nz The number of nonzeroes in the \a other container. + * @param[in] m The index dimension of the \a other container. + * @param[in] start The start position to copy from (inclusive). + * @param[in] end The end position to copy to (exclusive). + * + * The copy range is 2nz + m + 1, i.e., + * -# 0 <= start < 2nz + m + 1 + * -# 0 < end <= 2nz + m + 1 + * + * Concurrent calls to this function are allowed iff they consist of + * disjoint ranges \a start and \a end. The copy is guaranteed to be + * complete if the union of ranges spans 0 to 2nz + m + 1. + */ + template< + bool useId = false, + typename InputType, typename InputIND, typename InputSIZE + > void copyFrom( - const Compressed_Storage< D, IND, SIZE > &other, + const Compressed_Storage< InputType, InputIND, InputSIZE > &other, const size_t nz, const size_t m, const size_t start, size_t end, - const D * __restrict__ id = nullptr + const typename std::enable_if< !useId, void >::type * = nullptr ) { + static_assert( !std::is_void< InputType >::value, + "InputType must not be void" + ); #ifdef _DEBUG - std::cout << "CompressedStorage::copyFrom (no-cast) called with range " - << start << "--" << end; - if( use_id ) { - std::cout << ". The identity " << (*id) << " will be used.\n"; - } else { - std::cout << ". No identity will be used.\n"; - } + std::cout << "CompressedStorage::copyFrom called with range " + << start << "--" << end << ". No identity will be used.\n"; #endif size_t k = start; if( k < nz ) { @@ -632,39 +692,11 @@ namespace grb { std::cout << "\t value range " << k << " -- " << loop_end << "\n"; #endif assert( k <= loop_end ); - if( use_id ) { - std::fill( values + k, values + loop_end, *id ); - } else { - GRB_UTIL_IGNORE_CLASS_MEMACCESS // by the ALP spec, D can only be POD - // types. In this case raw memory copies - // are OK. - (void) std::memcpy( - values + k, - other.values + k, - (loop_end - k) * sizeof( D ) - ); - GRB_UTIL_RESTORE_WARNINGS - } - k = 0; - } else { - assert( k >= nz ); - k -= nz; - } - if( end <= nz ) { - return; - } - end -= nz; - if( k < nz ) { - const size_t loop_end = std::min( nz, end ); -#ifdef _DEBUG - std::cout << "\t index range " << k << " -- " << loop_end << "\n"; -#endif - assert( k <= loop_end ); - (void) std::memcpy( - row_index + k, - other.row_index + k, - (loop_end - k) * sizeof( IND ) - ); + + GRB_UTIL_IGNORE_CLASS_MEMACCESS; + std::copy_n( other.values + k, loop_end - k, values + k ); + GRB_UTIL_RESTORE_WARNINGS; + k = 0; } else { assert( k >= nz ); @@ -674,26 +706,12 @@ namespace grb { return; } end -= nz; - if( k < m + 1 ) { - const size_t loop_end = std::min( m + 1, end ); -#ifdef _DEBUG - std::cout << "\t start range " << k << " -- " << loop_end << "\n"; -#endif - assert( k <= loop_end ); - (void) std::memcpy( - col_start + k, - other.col_start + k, - (loop_end - k) * sizeof( SIZE ) - ); -#ifndef NDEBUG - for( size_t chk = k; chk < loop_end - 1; ++chk ) { - assert( other.col_start[ chk ] <= other.col_start[ chk + 1 ] ); - assert( col_start[ chk ] <= col_start[ chk + 1 ] ); - } -#endif - } + + copyCoordinatesFrom( *this, other, nz, m, k, end ); } + + /** * Writes a nonzero to the given position. Does \em not update the * \a col_start array. Does not perform any type checking. @@ -1198,45 +1216,22 @@ namespace grb { * \internal copyFrom specialisation for pattern matrices. */ template< - bool use_id = false, - typename InputType, - typename UnusedType = void + bool unusedValue = false, + typename InputType, typename InputIND, typename InputSIZE, + typename UnusedType = std::nullptr_t > void copyFrom( - const Compressed_Storage< InputType, IND, SIZE > &other, + const Compressed_Storage< InputType, InputIND, InputSIZE > &other, const size_t nz, const size_t m, const size_t start, size_t end, const UnusedType * __restrict__ = nullptr ) { // the use_id template is meaningless in the case of pattern matrices, but // is retained to keep the API the same as with the non-pattern case. - (void) use_id; #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom (void) called with range " << start << "--" << end << "\n"; #endif - size_t k = start; - if( k < nz ) { - const size_t loop_end = std::min( nz, end ); - (void) std::memcpy( - row_index + k, other.row_index + k, - (loop_end - k) * sizeof( IND ) - ); - k = 0; - } else { - assert( k >= nz ); - k -= nz; - } - if( end <= nz ) { - return; - } - end -= nz; - if( k < m + 1 ) { - const size_t loop_end = std::min( m + 1, end ); - (void) std::memcpy( - col_start + k, other.col_start + k, - (loop_end - k) * sizeof( SIZE ) - ); - } + copyCoordinatesFrom( *this, other, nz, m, start, end ); } /** diff --git a/include/graphblas/reference/io.hpp b/include/graphblas/reference/io.hpp index f6896e11b..b4ce47bdf 100644 --- a/include/graphblas/reference/io.hpp +++ b/include/graphblas/reference/io.hpp @@ -933,6 +933,11 @@ namespace grb { const Matrix< InputType1, reference, RIT, CIT, NIT > &A, const InputType2 * __restrict__ id = nullptr ) noexcept { +#ifndef NDEBUG + if( A_is_mask ) { + assert( id != nullptr ); + } +#endif #ifdef _DEBUG_REFERENCE_IO std::cout << "\t called grb::internal::set_copy (reference), " << "execute phase\n"; @@ -1020,30 +1025,18 @@ namespace grb { const size_t start = 0; size_t end = range; #endif - if( A_is_mask ) { - internal::getCRS( C ).template copyFrom< true >( - internal::getCRS( A ), nz, m, start, end, id - ); - } else { - internal::getCRS( C ).template copyFrom< false >( - internal::getCRS( A ), nz, m, start, end - ); - } + internal::getCRS( C ).template copyFrom< A_is_mask >( + internal::getCRS( A ), nz, m, start, end, id + ); range = internal::getCCS( C ).copyFromRange( nz, n ); #ifdef _H_GRB_REFERENCE_OMP_IO config::OMP::localRange( start, end, 0, range ); #else end = range; #endif - if( A_is_mask ) { - internal::getCCS( C ).template copyFrom< true >( - internal::getCCS( A ), nz, n, start, end, id - ); - } else { - internal::getCCS( C ).template copyFrom< false >( - internal::getCCS( A ), nz, n, start, end - ); - } + internal::getCCS( C ).template copyFrom< A_is_mask >( + internal::getCCS( A ), nz, n, start, end, id + ); } internal::setCurrentNonzeroes( C, nz ); @@ -1835,8 +1828,8 @@ namespace grb { !grb::is_object< InputType >::value, void >::type * const = nullptr ) noexcept { - static_assert( std::is_same< OutputType, void >::value || - !std::is_same< InputType, void >::value, + static_assert( + !std::is_void< InputType >::value || std::is_same< OutputType, InputType >::value, "grb::set cannot interpret an input pattern matrix without a " "semiring or a monoid. This interpretation is needed for " "writing the non-pattern matrix output. Possible solutions: 1) " @@ -1846,7 +1839,8 @@ namespace grb { std::cout << "Called grb::set (matrix-to-matrix, reference)" << std::endl; #endif // static checks - NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || + NO_CAST_ASSERT( + ( !(descr & descriptors::no_casting) || std::is_same< InputType, OutputType >::value ), "grb::set", "called with non-matching value types" ); @@ -1906,6 +1900,13 @@ namespace grb { std::cout << "Called grb::set (matrix-to-value-masked, reference)\n"; #endif // static checks + static_assert( std::is_void< OutputType >::value || + std::is_same< OutputType, InputType2 >::value || + std::is_convertible< InputType2, OutputType >::value, + "grb::set (masked set to value): non-void output type should be either a) " + "the same as the input scalar value type or b) the input scalar type should " + "be convertible to the output type" + ); NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || std::is_same< InputType2, OutputType >::value ), From 7a1bfc93712544fc12e5948d51b5ca82d09fc65c Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Thu, 31 Aug 2023 10:26:32 +0200 Subject: [PATCH 03/19] Declaration of grb::set matrix variants in base --- include/graphblas/base/io.hpp | 25 +++++++++++++------------ include/graphblas/nonblocking/io.hpp | 9 +++++---- include/graphblas/reference/io.hpp | 15 ++++++++++++--- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/include/graphblas/base/io.hpp b/include/graphblas/base/io.hpp index 0c0bab768..250587748 100644 --- a/include/graphblas/base/io.hpp +++ b/include/graphblas/base/io.hpp @@ -1181,13 +1181,14 @@ namespace grb { */ template< Descriptor descr = descriptors::no_operation, - typename OutputType, typename RIT, typename CIT, typename NIT, - typename InputType, + typename OutputType, typename InputType, + typename RIT1, typename CIT1, typename NIT1, + typename RIT2, typename CIT2, typename NIT2, Backend backend > RC set( - Matrix< OutputType, backend, RIT, CIT, NIT > &C, - const Matrix< InputType, backend, RIT, CIT, NIT > &A, + Matrix< OutputType, backend, RIT1, CIT1, NIT1 > &A, + const Matrix< InputType, backend, RIT2, CIT2, NIT2 > &C, const Phase &phase = EXECUTE, const typename std::enable_if< !grb::is_object< OutputType >::value && @@ -1198,8 +1199,8 @@ namespace grb { const bool should_not_call_base_matrix_set = false; assert( should_not_call_base_matrix_set ); #endif - (void) C; (void) A; + (void) C; (void) phase; return UNSUPPORTED; } @@ -1292,15 +1293,15 @@ namespace grb { */ template< Descriptor descr = descriptors::no_operation, - typename DataType, typename RIT, typename CIT, typename NIT, - typename MaskType, - typename ValueType, + typename OutputType, typename InputType, typename ValueType, + typename RIT1, typename CIT1, typename NIT1, + typename RIT2, typename CIT2, typename NIT2, Backend backend > RC set( - Matrix< DataType, backend, RIT, CIT, NIT > &C, - const Matrix< MaskType, backend, RIT, CIT, NIT > &mask, - const ValueType& val, + Matrix< OutputType, backend, RIT1, CIT1, NIT1 > &C, + const Matrix< InputType, backend, RIT2, CIT2, NIT2 > &A, + const ValueType &val, const Phase &phase = EXECUTE, const typename std::enable_if< !grb::is_object< DataType >::value && @@ -1312,8 +1313,8 @@ namespace grb { const bool should_not_call_base_matrix_masked_set = false; assert( should_not_call_base_matrix_masked_set ); #endif + (void) A; (void) C; - (void) mask; (void) val; (void) phase; return UNSUPPORTED; diff --git a/include/graphblas/nonblocking/io.hpp b/include/graphblas/nonblocking/io.hpp index beaaf4df5..1b6b65630 100644 --- a/include/graphblas/nonblocking/io.hpp +++ b/include/graphblas/nonblocking/io.hpp @@ -1171,11 +1171,12 @@ namespace grb { template< Descriptor descr = descriptors::no_operation, typename OutputType, typename InputType1, typename InputType2, - typename RIT, typename CIT, typename NIT + typename RIT1, typename CIT1, typename NIT1, + typename RIT2, typename CIT2, typename NIT2 > RC set( - Matrix< OutputType, nonblocking, RIT, CIT, NIT > &C, - const Matrix< InputType1, nonblocking, RIT, CIT, NIT > &A, + Matrix< OutputType, nonblocking, RIT1, CIT1, NIT1 > &C, + const Matrix< InputType, nonblocking, RIT2, CIT2, NIT2 > &A, const InputType2 &val, const Phase &phase = EXECUTE, const typename std::enable_if< @@ -1189,7 +1190,7 @@ namespace grb { #endif // static checks NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || - std::is_same< InputType2, OutputType >::value + std::is_same< ValueType, OutputType >::value ), "grb::set", "called with non-matching value types" ); diff --git a/include/graphblas/reference/io.hpp b/include/graphblas/reference/io.hpp index b4ce47bdf..071e6a085 100644 --- a/include/graphblas/reference/io.hpp +++ b/include/graphblas/reference/io.hpp @@ -1883,11 +1883,12 @@ namespace grb { template< Descriptor descr = descriptors::no_operation, typename OutputType, typename InputType1, typename InputType2, - typename RIT, typename CIT, typename NIT + typename RIT1, typename CIT1, typename NIT1, + typename RIT2, typename CIT2, typename NIT2 > RC set( - Matrix< OutputType, reference, RIT, CIT, NIT > &C, - const Matrix< InputType1, reference, RIT, CIT, NIT > &A, + Matrix< OutputType, reference, RIT1, CIT1, NIT1 > &C, + const Matrix< InputType1, reference, RIT2, CIT2, NIT2 > &A, const InputType2 &val, const Phase &phase = EXECUTE, const typename std::enable_if< @@ -1900,6 +1901,14 @@ namespace grb { std::cout << "Called grb::set (matrix-to-value-masked, reference)\n"; #endif // static checks + static_assert( !std::is_void< OutputType >::value, + "internal::grb::set (masked set to value): cannot have a pattern " + "matrix as output" + ); + static_assert( std::is_convertible< ValueType, OutputType >::value, + "internal::grb::set (masked set to value): value type cannot be " + "converted to output type" + ); static_assert( std::is_void< OutputType >::value || std::is_same< OutputType, InputType2 >::value || std::is_convertible< InputType2, OutputType >::value, From d4596533eeace7f1a1a940e1ee70bbca7aa93adf Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Mon, 5 Feb 2024 10:59:36 +0100 Subject: [PATCH 04/19] Merge bugfix --- tests/unit/copyMixedDomainsMatrices.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/copyMixedDomainsMatrices.cpp b/tests/unit/copyMixedDomainsMatrices.cpp index e87698f8f..a5c3d05ad 100644 --- a/tests/unit/copyMixedDomainsMatrices.cpp +++ b/tests/unit/copyMixedDomainsMatrices.cpp @@ -21,7 +21,6 @@ #include #include -#include using namespace grb; From a2d4a64b51ea01f32f5058a431deeb79af7f4422 Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Mon, 5 Feb 2024 16:04:14 +0100 Subject: [PATCH 05/19] Review fixes --- .../reference/compressed_storage.hpp | 47 ++++++++++++------- tests/unit/copyMixedDomainsMatrices.cpp | 7 +-- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 37883845f..ee6171f30 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -25,6 +25,11 @@ #include //std::memcpy +#if reference == reference_omp + #define _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #include +#endif + namespace grb { @@ -76,11 +81,12 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - std::copy_n( - input.row_index + k, - loop_end - k, - output.row_index + k - ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + output.row_index[ i ] = static_cast< OutputIND >( input.row_index[ i ] ); + } k = 0; } else { assert( k >= nz ); @@ -93,11 +99,12 @@ namespace grb { if( k < m + 1 ) { const size_t loop_end = std::min( m + 1, end ); assert( k <= loop_end ); - std::copy_n( - input.col_start + k, - loop_end - k, - output.col_start + k - ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + output.col_start[ i ] = static_cast< OutputSIZE >( input.col_start[ i ] ); + } #ifndef NDEBUG for( size_t chk = k; chk < loop_end - 1; ++chk ) { assert( input.col_start[ chk ] <= input.col_start[ chk + 1 ] ); @@ -599,13 +606,16 @@ namespace grb { * * Performs no safety checking. Performs no (re-)allocations. * + * @tparam use_id If set to true, use \a id instead of values in + * \a other. * @param[in] other The container to copy from. * @param[in] nz The number of nonzeroes in the \a other container. * @param[in] m The index dimension of the \a other container. * @param[in] start The start position to copy from (inclusive). * @param[in] end The end position to copy to (exclusive). * @param[in] id A pointer to a value overriding those in \a other. - * + * Will only be used if and only if \a use_id is set + * to true. * The copy range is 2nz + m + 1, i.e., * -# 0 <= start < 2nz + m + 1 * -# 0 < end <= 2nz + m + 1 @@ -694,7 +704,12 @@ namespace grb { assert( k <= loop_end ); GRB_UTIL_IGNORE_CLASS_MEMACCESS; - std::copy_n( other.values + k, loop_end - k, values + k ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + values[ i ] = static_cast< D >( other.values[ i ] ); + } GRB_UTIL_RESTORE_WARNINGS; k = 0; @@ -710,8 +725,6 @@ namespace grb { copyCoordinatesFrom( *this, other, nz, m, k, end ); } - - /** * Writes a nonzero to the given position. Does \em not update the * \a col_start array. Does not perform any type checking. @@ -1225,8 +1238,10 @@ namespace grb { const size_t nz, const size_t m, const size_t start, size_t end, const UnusedType * __restrict__ = nullptr ) { - // the use_id template is meaningless in the case of pattern matrices, but - // is retained to keep the API the same as with the non-pattern case. + (void) unusedValue; + // the unusedValue template is meaningless in the case of + // pattern matrices, but is retained to keep the API + // the same as with the non-pattern case. #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom (void) called with range " << start << "--" << end << "\n"; diff --git a/tests/unit/copyMixedDomainsMatrices.cpp b/tests/unit/copyMixedDomainsMatrices.cpp index a5c3d05ad..290223045 100644 --- a/tests/unit/copyMixedDomainsMatrices.cpp +++ b/tests/unit/copyMixedDomainsMatrices.cpp @@ -22,7 +22,6 @@ #include - using namespace grb; @@ -54,8 +53,6 @@ void grb_program( const size_t &n, grb::RC &rc ) { } } - - { // Try cast to ushort (should succeed) Matrix< ushort > M_short( n, n, 0UL ); rc = grb::set( M_short, A_int, Phase::RESIZE ); @@ -176,10 +173,10 @@ int main( int argc, char ** argv ) { if( argc == 2 ) { size_t read; std::istringstream ss( argv[ 1 ] ); - if( ! ( ss >> read ) ) { + if( !( ss >> read ) ) { std::cerr << "Error parsing first argument\n"; printUsage = true; - } else if( ! ss.eof() ) { + } else if( !ss.eof() ) { std::cerr << "Error parsing first argument\n"; printUsage = true; } else { From 507f06f39ab5d8a0d5c9135d705d68fcbdb8d2b3 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Wed, 7 Feb 2024 17:55:16 +0100 Subject: [PATCH 06/19] Small typo fix --- include/graphblas/reference/compressed_storage.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index ee6171f30..8d43d2bf5 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -105,12 +105,12 @@ namespace grb { for( size_t i = k; i < loop_end; ++i ) { output.col_start[ i ] = static_cast< OutputSIZE >( input.col_start[ i ] ); } - #ifndef NDEBUG +#ifndef NDEBUG for( size_t chk = k; chk < loop_end - 1; ++chk ) { assert( input.col_start[ chk ] <= input.col_start[ chk + 1 ] ); assert( output.col_start[ chk ] <= output.col_start[ chk + 1 ] ); } - #endif +#endif } } } // namespace From 41e331c8c888a32754c27c13138464ddc3aa00c9 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Thu, 15 Feb 2024 14:41:10 +0100 Subject: [PATCH 07/19] Clarify specification for masked matrix set --- include/graphblas/base/io.hpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/include/graphblas/base/io.hpp b/include/graphblas/base/io.hpp index 250587748..4e835413f 100644 --- a/include/graphblas/base/io.hpp +++ b/include/graphblas/base/io.hpp @@ -1279,6 +1279,10 @@ namespace grb { * is forbidden also. Implementations shall throw a static assertion failure * if the user nonetheless asks for structural mask inversion. * + * \warning Mask-modifier descriptors, such as #grb::descriptors::invert_mask + * are not accepted while #grb::descriptors::structural is in fact + * implied by this specification. + * * \parblock * \par Performance semantics * Each backend must define performance semantics for this primitive. @@ -1300,7 +1304,7 @@ namespace grb { > RC set( Matrix< OutputType, backend, RIT1, CIT1, NIT1 > &C, - const Matrix< InputType, backend, RIT2, CIT2, NIT2 > &A, + const Matrix< InputType, backend, RIT2, CIT2, NIT2 > &mask, const ValueType &val, const Phase &phase = EXECUTE, const typename std::enable_if< @@ -1313,8 +1317,8 @@ namespace grb { const bool should_not_call_base_matrix_masked_set = false; assert( should_not_call_base_matrix_masked_set ); #endif - (void) A; (void) C; + (void) mask; (void) val; (void) phase; return UNSUPPORTED; From 844d728085daf6d822ca15ebb0f31cfc1a013442 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Thu, 15 Feb 2024 14:41:21 +0100 Subject: [PATCH 08/19] Fix minor style issue --- include/graphblas/reference/compressed_storage.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 8d43d2bf5..7dc3e7997 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -26,8 +26,8 @@ #include //std::memcpy #if reference == reference_omp - #define _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #include + #define _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #include #endif From f91c719c8d9fc861807afd8da808904f4ec32325 Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Fri, 16 Feb 2024 11:42:48 +0100 Subject: [PATCH 09/19] Review partial fixes --- include/graphblas/reference/compressed_storage.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 7dc3e7997..17b1298f5 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -47,11 +47,11 @@ namespace grb { * Performs no safety checking. Performs no (re-)allocations. * * @param[out] output The container to copy the coordinates to. - * @param[in] input The container to copy the coordinates from. - * @param[in] nz The number of nonzeroes in the \a other container. - * @param[in] m The index dimension of the \a other container. - * @param[in] k The start position to copy from (inclusive). - * @param[in] end The end position to copy to (exclusive). + * @param[in] input The container to copy the coordinates from. + * @param[in] nz The number of nonzeroes in the \a other container. + * @param[in] m The index dimension of the \a other container. + * @param[in] k The start position to copy from (inclusive). + * @param[in] end The end position to copy to (exclusive). * * The copy range is 2nz + m + 1, i.e., * -# 0 <= start < 2nz + m + 1 From cbfc495c22c904c0f935409173931789498d3e36 Mon Sep 17 00:00:00 2001 From: Albert-Jan Yzelman Date: Mon, 19 Feb 2024 09:35:55 +0100 Subject: [PATCH 10/19] Code review: reinstate lost rationale on ignoring static warnings --- include/graphblas/reference/compressed_storage.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 17b1298f5..2116588ff 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -703,7 +703,9 @@ namespace grb { #endif assert( k <= loop_end ); - GRB_UTIL_IGNORE_CLASS_MEMACCESS; + GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD + // type, in which case raw memory copies + // are OK #ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE #pragma omp for simd #endif From e7a63f4ff8cd6a6385746c9907b23f7ea33f6128 Mon Sep 17 00:00:00 2001 From: Benjamin Lozes Date: Wed, 28 Feb 2024 16:56:47 +0100 Subject: [PATCH 11/19] Final implementation --- .../reference/compressed_storage.hpp | 247 ++++++++++++------ include/graphblas/reference/io.hpp | 38 ++- 2 files changed, 187 insertions(+), 98 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 2116588ff..a6114ee60 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -38,83 +38,6 @@ namespace grb { template< typename D, typename IND, typename SIZE > class Compressed_Storage; - namespace { - - /** - * Copies row_index and col_index from a - * given Compressed_Storage to another. - * - * Performs no safety checking. Performs no (re-)allocations. - * - * @param[out] output The container to copy the coordinates to. - * @param[in] input The container to copy the coordinates from. - * @param[in] nz The number of nonzeroes in the \a other container. - * @param[in] m The index dimension of the \a other container. - * @param[in] k The start position to copy from (inclusive). - * @param[in] end The end position to copy to (exclusive). - * - * The copy range is 2nz + m + 1, i.e., - * -# 0 <= start < 2nz + m + 1 - * -# 0 < end <= 2nz + m + 1 - * - * Concurrent calls to this function are allowed iff they consist of - * disjoint ranges \a start and \a end. The copy is guaranteed to be - * complete if the union of ranges spans 0 to 2nz + m + 1. - */ - template< - typename OutputType, typename OutputIND, typename OutputSIZE, - typename InputType, typename InputIND, typename InputSIZE - > - static inline void copyCoordinatesFrom( - Compressed_Storage< OutputType, OutputIND, OutputSIZE > &output, - const Compressed_Storage< InputType, InputIND, InputSIZE > &input, - const size_t nz, const size_t m, - size_t k, size_t end - ) { - static_assert( std::is_convertible< InputIND, OutputIND >::value, - "InputIND must be convertible to OutputIND" - ); - static_assert( std::is_convertible< InputSIZE, OutputSIZE >::value, - "InputSIZE must be convertible to OutputSIZE" - ); - - if( k < nz ) { - const size_t loop_end = std::min( nz, end ); - assert( k <= loop_end ); -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif - for( size_t i = k; i < loop_end; ++i ) { - output.row_index[ i ] = static_cast< OutputIND >( input.row_index[ i ] ); - } - k = 0; - } else { - assert( k >= nz ); - k -= nz; - } - if( end <= nz ) { - return; - } - end -= nz; - if( k < m + 1 ) { - const size_t loop_end = std::min( m + 1, end ); - assert( k <= loop_end ); -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif - for( size_t i = k; i < loop_end; ++i ) { - output.col_start[ i ] = static_cast< OutputSIZE >( input.col_start[ i ] ); - } -#ifndef NDEBUG - for( size_t chk = k; chk < loop_end - 1; ++chk ) { - assert( input.col_start[ chk ] <= input.col_start[ chk + 1 ] ); - assert( output.col_start[ chk ] <= output.col_start[ chk + 1 ] ); - } -#endif - } - } - } // namespace - /** * Basic functionality for a compressed storage format (CRS/CSR or CCS/CSC). * @@ -625,6 +548,7 @@ namespace grb { * complete if the union of ranges spans 0 to 2nz + m + 1. */ template< + Descriptor descr = descriptors::no_operation, bool useId = true, typename InputType, typename InputIND, typename InputSIZE, typename ValueType @@ -636,6 +560,16 @@ namespace grb { const ValueType * __restrict__ id, const typename std::enable_if< useId, void >::type * = nullptr ) { + static_assert( + ( useId && std::is_convertible< ValueType, D >::value ), + "ValueType must be convertible to D" + ); + static_assert( std::is_convertible< InputIND, IND >::value, + "InputIND must be convertible to IND" + ); + static_assert( std::is_convertible< InputSIZE, SIZE >::value, + "InputSIZE must be convertible to SIZE" + ); #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom (cast) called with range " << start << "--" << end << ". The identity " << (*id) << " will be used.\n"; @@ -645,7 +579,47 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - std::fill_n( values + k, loop_end - k, *id ); + GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD + // type, in which case raw memory copies + // are OK +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + if( utils::interpretMatrixMask< descr, InputType >( + true, other.getValues(), i ) + ) { + values[ i ] = static_cast< D >( *id ); + } + } + GRB_UTIL_RESTORE_WARNINGS; + k = 0; + } else { + assert( k >= nz ); + k -= nz; + } + if( end <= nz ) { + return; + } + end -= nz; + + if( k < nz ) { + const size_t loop_end = std::min( nz, end ); + assert( k <= loop_end ); + GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD + // type, in which case raw memory copies + // are OK +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + if( utils::interpretMatrixMask< descr, InputType >( + true, other.getValues(), i ) + ) { + row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); + } + } + GRB_UTIL_RESTORE_WARNINGS; k = 0; } else { assert( k >= nz ); @@ -656,7 +630,30 @@ namespace grb { } end -= nz; - copyCoordinatesFrom( *this, other, nz, m, k, end ); + if( k < m + 1 ) { + const size_t loop_end = std::min( m + 1, end ); + assert( k <= loop_end ); + GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD + // type, in which case raw memory copies + // are OK +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + if( utils::interpretMatrixMask< descr, InputType >( + true, other.getValues(), i ) + ) { + col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); + } + } + GRB_UTIL_RESTORE_WARNINGS; +#ifndef NDEBUG + for( size_t chk = k; chk < loop_end - 1; ++chk ) { + assert( other.col_start[ chk ] <= col_start[ chk + 1 ] ); + assert( col_start[ chk ] <= col_start[ chk + 1 ] ); + } +#endif + } } /** @@ -679,6 +676,7 @@ namespace grb { * complete if the union of ranges spans 0 to 2nz + m + 1. */ template< + Descriptor descr = descriptors::no_operation, bool useId = false, typename InputType, typename InputIND, typename InputSIZE > @@ -691,6 +689,16 @@ namespace grb { static_assert( !std::is_void< InputType >::value, "InputType must not be void" ); + static_assert( + ( !useId && std::is_convertible< InputType, D >::value ), + "InputType must be convertible to D" + ); + static_assert( std::is_convertible< InputIND, IND >::value, + "InputIND must be convertible to IND" + ); + static_assert( std::is_convertible< InputSIZE, SIZE >::value, + "InputSIZE must be convertible to SIZE" + ); #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom called with range " << start << "--" << end << ". No identity will be used.\n"; @@ -698,14 +706,10 @@ namespace grb { size_t k = start; if( k < nz ) { const size_t loop_end = std::min( nz, end ); -#ifdef _DEBUG - std::cout << "\t value range " << k << " -- " << loop_end << "\n"; -#endif assert( k <= loop_end ); - GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD // type, in which case raw memory copies - // are OK + // are OK #ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE #pragma omp for simd #endif @@ -724,7 +728,41 @@ namespace grb { } end -= nz; - copyCoordinatesFrom( *this, other, nz, m, k, end ); + if( k < nz ) { + const size_t loop_end = std::min( nz, end ); + assert( k <= loop_end ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); + } + k = 0; + } else { + assert( k >= nz ); + k -= nz; + } + if( end <= nz ) { + return; + } + end -= nz; + + if( k < m + 1 ) { + const size_t loop_end = std::min( m + 1, end ); + assert( k <= loop_end ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); + } +#ifndef NDEBUG + for( size_t chk = k; chk < loop_end - 1; ++chk ) { + assert( other.col_start[ chk ] <= other.col_start[ chk + 1 ] ); + assert( col_start[ chk ] <= col_start[ chk + 1 ] ); + } +#endif + } } /** @@ -1231,6 +1269,7 @@ namespace grb { * \internal copyFrom specialisation for pattern matrices. */ template< + Descriptor = descriptors::no_operation, bool unusedValue = false, typename InputType, typename InputIND, typename InputSIZE, typename UnusedType = std::nullptr_t @@ -1244,11 +1283,47 @@ namespace grb { // the unusedValue template is meaningless in the case of // pattern matrices, but is retained to keep the API // the same as with the non-pattern case. + auto k = start; #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom (void) called with range " << start << "--" << end << "\n"; #endif - copyCoordinatesFrom( *this, other, nz, m, start, end ); + + if( k < nz ) { + const size_t loop_end = std::min( nz, end ); + assert( k <= loop_end ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); + } + k = 0; + } else { + assert( k >= nz ); + k -= nz; + } + if( end <= nz ) { + return; + } + end -= nz; + + if( k < m + 1 ) { + const size_t loop_end = std::min( m + 1, end ); + assert( k <= loop_end ); +#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE + #pragma omp for simd +#endif + for( size_t i = k; i < loop_end; ++i ) { + col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); + } +#ifndef NDEBUG + for( size_t chk = k; chk < loop_end - 1; ++chk ) { + assert( other.col_start[ chk ] <= other.col_start[ chk + 1 ] ); + assert( col_start[ chk ] <= col_start[ chk + 1 ] ); + } +#endif + } } /** diff --git a/include/graphblas/reference/io.hpp b/include/graphblas/reference/io.hpp index 071e6a085..56d0b7ae0 100644 --- a/include/graphblas/reference/io.hpp +++ b/include/graphblas/reference/io.hpp @@ -1025,16 +1025,16 @@ namespace grb { const size_t start = 0; size_t end = range; #endif - internal::getCRS( C ).template copyFrom< A_is_mask >( + internal::getCRS( C ).template copyFrom< descr, A_is_mask >( internal::getCRS( A ), nz, m, start, end, id ); - range = internal::getCCS( C ).copyFromRange( nz, n ); + range = internal::getCCS( C ).copyFromRange( descr, nz, n ); #ifdef _H_GRB_REFERENCE_OMP_IO config::OMP::localRange( start, end, 0, range ); #else end = range; #endif - internal::getCCS( C ).template copyFrom< A_is_mask >( + internal::getCCS( C ).template copyFrom< descr, A_is_mask >( internal::getCCS( A ), nz, n, start, end, id ); @@ -1699,6 +1699,14 @@ namespace grb { #ifdef _DEBUG std::cout << "In grb::set (vector-to-value, masked)\n"; #endif + static_assert( + std::is_void< MaskType >::value || + (descr & descriptors::structural) || + std::is_convertible< MaskType, bool > ::value, + "grb::set (masked set to value): mask vector must be a " + "pattern vector, or have a data-type that is convertible to bool, " + "or use the structural descriptor" + ); // static sanity checks NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || std::is_same< DataType, T >::value ), "grb::set (Vector to scalar, masked)", @@ -1829,7 +1837,8 @@ namespace grb { void >::type * const = nullptr ) noexcept { static_assert( - !std::is_void< InputType >::value || std::is_same< OutputType, InputType >::value, + !std::is_void< InputType >::value || + std::is_same< OutputType, InputType >::value, "grb::set cannot interpret an input pattern matrix without a " "semiring or a monoid. This interpretation is needed for " "writing the non-pattern matrix output. Possible solutions: 1) " @@ -1901,14 +1910,6 @@ namespace grb { std::cout << "Called grb::set (matrix-to-value-masked, reference)\n"; #endif // static checks - static_assert( !std::is_void< OutputType >::value, - "internal::grb::set (masked set to value): cannot have a pattern " - "matrix as output" - ); - static_assert( std::is_convertible< ValueType, OutputType >::value, - "internal::grb::set (masked set to value): value type cannot be " - "converted to output type" - ); static_assert( std::is_void< OutputType >::value || std::is_same< OutputType, InputType2 >::value || std::is_convertible< InputType2, OutputType >::value, @@ -1916,6 +1917,19 @@ namespace grb { "the same as the input scalar value type or b) the input scalar type should " "be convertible to the output type" ); + static_assert( + std::is_void< InputType1 >::value || + std::is_convertible< InputType1, bool >::value, + "grb::set (masked set to value): mask matrix must be a " + "pattern matrix or have a data-type that is convertible to bool" + ); + static_assert( !( + ( descr & descriptors::structural ) && + ( descr & descriptors::invert_mask) + ), + "grb::set (masked set to value): descriptors::structural " + "and descriptors::invert_mask cannot be combined" + ); NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || std::is_same< InputType2, OutputType >::value ), From 76284d9cf3e56d7ab6dab01084414b60a722bbe1 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Wed, 29 Jan 2025 13:32:14 +0100 Subject: [PATCH 12/19] Fix rebase issues --- include/graphblas/base/io.hpp | 16 ++++++++-------- include/graphblas/nonblocking/io.hpp | 4 ++-- include/graphblas/reference/io.hpp | 20 +++++++++----------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/include/graphblas/base/io.hpp b/include/graphblas/base/io.hpp index 4e835413f..265988cf1 100644 --- a/include/graphblas/base/io.hpp +++ b/include/graphblas/base/io.hpp @@ -1223,10 +1223,10 @@ namespace grb { * mutually exclusive for this primitive. * \endparblock * - * @tparam DataType The type of each element in the given matrix. - * @tparam MaskType The type of each element in the given mask. - * @tparam ValueType The type of the given value. Should be convertible - * to \a DataType. + * @tparam OutputType The type of each element in the given matrix. + * @tparam MaskType The type of each element in the given mask. + * @tparam ValueType The type of the given value. Should be convertible + * to \a OutputType. * @tparam RIT The integer type for encoding row indices. * @tparam CIT The integer type for encoding column indices. * @tparam NIT The integer type for encoding nonzero indices. @@ -1265,7 +1265,7 @@ namespace grb { * * When \a descr includes #grb::descriptors::no_casting then code shall not * compile if one of the following conditions are met: - * -# \a ValueType does not match \a DataType; or + * -# \a ValueType does not match \a OutputType; or * -# \a MaskType does not match bool. * * In these cases, the code shall not compile: implementations must throw @@ -1297,18 +1297,18 @@ namespace grb { */ template< Descriptor descr = descriptors::no_operation, - typename OutputType, typename InputType, typename ValueType, + typename OutputType, typename MaskType, typename ValueType, typename RIT1, typename CIT1, typename NIT1, typename RIT2, typename CIT2, typename NIT2, Backend backend > RC set( Matrix< OutputType, backend, RIT1, CIT1, NIT1 > &C, - const Matrix< InputType, backend, RIT2, CIT2, NIT2 > &mask, + const Matrix< MaskType, backend, RIT2, CIT2, NIT2 > &mask, const ValueType &val, const Phase &phase = EXECUTE, const typename std::enable_if< - !grb::is_object< DataType >::value && + !grb::is_object< OutputType >::value && !grb::is_object< ValueType >::value && !grb::is_object< MaskType >::value, void >::type * const = nullptr diff --git a/include/graphblas/nonblocking/io.hpp b/include/graphblas/nonblocking/io.hpp index 1b6b65630..0831fc564 100644 --- a/include/graphblas/nonblocking/io.hpp +++ b/include/graphblas/nonblocking/io.hpp @@ -1176,7 +1176,7 @@ namespace grb { > RC set( Matrix< OutputType, nonblocking, RIT1, CIT1, NIT1 > &C, - const Matrix< InputType, nonblocking, RIT2, CIT2, NIT2 > &A, + const Matrix< InputType1, nonblocking, RIT2, CIT2, NIT2 > &A, const InputType2 &val, const Phase &phase = EXECUTE, const typename std::enable_if< @@ -1190,7 +1190,7 @@ namespace grb { #endif // static checks NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) || - std::is_same< ValueType, OutputType >::value + std::is_same< InputType2, OutputType >::value ), "grb::set", "called with non-matching value types" ); diff --git a/include/graphblas/reference/io.hpp b/include/graphblas/reference/io.hpp index 56d0b7ae0..f4f1b0709 100644 --- a/include/graphblas/reference/io.hpp +++ b/include/graphblas/reference/io.hpp @@ -925,13 +925,13 @@ namespace grb { bool A_is_mask, Descriptor descr, typename OutputType, typename InputType1, - typename InputType2 = const OutputType, + typename InputType2, typename RIT, typename CIT, typename NIT > RC set_copy( Matrix< OutputType, reference, RIT, CIT, NIT > &C, const Matrix< InputType1, reference, RIT, CIT, NIT > &A, - const InputType2 * __restrict__ id = nullptr + const InputType2 * __restrict__ id ) noexcept { #ifndef NDEBUG if( A_is_mask ) { @@ -1028,7 +1028,7 @@ namespace grb { internal::getCRS( C ).template copyFrom< descr, A_is_mask >( internal::getCRS( A ), nz, m, start, end, id ); - range = internal::getCCS( C ).copyFromRange( descr, nz, n ); + range = internal::getCCS( C ).copyFromRange( nz, n ); #ifdef _H_GRB_REFERENCE_OMP_IO config::OMP::localRange( start, end, 0, range ); #else @@ -1885,7 +1885,8 @@ namespace grb { return grb::resize( C, std::max( nnz( C ), nnz( A ) ) ); } else { assert( phase == EXECUTE ); - return internal::set_copy< false, descr >( C, A ); + const OutputType * const dummy = nullptr; + return internal::set_copy< false, descr >( C, A, dummy ); } } @@ -2020,13 +2021,10 @@ namespace grb { std::cout << "\t dispatching to void or non-void set_copy variant\n"; #endif assert( phase == EXECUTE ); - if( std::is_same< OutputType, void >::value ) { - return internal::set_copy< false, descr & ~(descriptors::invert_mask) >( - C, A ); - } else { - return internal::set_copy< true, descr & ~(descriptors::invert_mask) >( - C, A, &val ); - } + constexpr bool outputIsVoid = std::is_void< OutputType >::value; + return internal::set_copy< + !outputIsVoid, descr & ~(descriptors::invert_mask) + >( C, A, &val ); } } From 8ddb53f7a8676e5126410ae8aa5b35accf8ccbb6 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Wed, 29 Jan 2025 13:38:39 +0100 Subject: [PATCH 13/19] Comment from MR no longer applies on current develop- removed --- include/graphblas/base/io.hpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/include/graphblas/base/io.hpp b/include/graphblas/base/io.hpp index 265988cf1..493dfcfea 100644 --- a/include/graphblas/base/io.hpp +++ b/include/graphblas/base/io.hpp @@ -1279,10 +1279,6 @@ namespace grb { * is forbidden also. Implementations shall throw a static assertion failure * if the user nonetheless asks for structural mask inversion. * - * \warning Mask-modifier descriptors, such as #grb::descriptors::invert_mask - * are not accepted while #grb::descriptors::structural is in fact - * implied by this specification. - * * \parblock * \par Performance semantics * Each backend must define performance semantics for this primitive. From bf56cd3c5f4ac66258283ced93d5b3645e48dc42 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Wed, 29 Jan 2025 13:50:28 +0100 Subject: [PATCH 14/19] Code review: avoid using default template args, clearly mark internal static_asserts as internal --- .../graphblas/reference/compressed_storage.hpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index a6114ee60..17502e9ce 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -548,8 +548,8 @@ namespace grb { * complete if the union of ranges spans 0 to 2nz + m + 1. */ template< - Descriptor descr = descriptors::no_operation, - bool useId = true, + Descriptor descr, + bool useId, typename InputType, typename InputIND, typename InputSIZE, typename ValueType > @@ -562,17 +562,21 @@ namespace grb { ) { static_assert( ( useId && std::is_convertible< ValueType, D >::value ), - "ValueType must be convertible to D" + "internal logic error: ValueType must be convertible to D. Please submit " + "a bug report" ); static_assert( std::is_convertible< InputIND, IND >::value, - "InputIND must be convertible to IND" + "internal logic error: InputIND must be convertible to IND. " + "Please submit a bug report" ); static_assert( std::is_convertible< InputSIZE, SIZE >::value, - "InputSIZE must be convertible to SIZE" + "internal logic error: InputSIZE must be convertible to SIZE. " + "Please submit a bug report" ); #ifdef _DEBUG std::cout << "CompressedStorage::copyFrom (cast) called with range " - << start << "--" << end << ". The identity " << (*id) << " will be used.\n"; + << start << "--" << end << ". The identity " << (*id) + << " will be used.\n"; #endif assert( start <= end ); size_t k = start; From b86203c115cc09111dd8b9b59155346aa3ddf337 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Wed, 29 Jan 2025 14:04:40 +0100 Subject: [PATCH 15/19] Minor code style fixes re final code review --- .../reference/compressed_storage.hpp | 68 +++++++++++-------- include/graphblas/reference/matrix.hpp | 6 +- 2 files changed, 42 insertions(+), 32 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 17502e9ce..fd3204947 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -30,6 +30,10 @@ #include #endif +#ifdef _DEBUG + #define _DEBUG_REFERENCE_COMPRESSED_STORAGE +#endif + namespace grb { @@ -152,7 +156,7 @@ namespace grb { k( 0 ), m( 0 ), n( 0 ), row( 1 ), s( 0 ), P( 1 ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator default constructor (generic) called\n"; #endif nonzero.first.first = 1; @@ -167,7 +171,7 @@ namespace grb { row( other.row ), s( other.s ), P( other.P ), nonzero( other.nonzero ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Matrix< reference >::const_iterator copy-constructor " << "called\n"; #endif @@ -175,7 +179,7 @@ namespace grb { /** Move constructor. */ ConstIterator( ConstIterator &&other ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Matrix< reference >::const_iterator move-constructor " << "called\n"; #endif @@ -202,7 +206,7 @@ namespace grb { m( _m ), n( _n ), s( _s ), P( _P ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Compressed_Storage::Const_Iterator constructor called, " << "with storage " << ( &_storage ) << ", " << "m " << _m << ", n " << _n << ", and end " << end << ".\n"; @@ -220,7 +224,7 @@ namespace grb { } if( row < m ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "\tInitial pair, pre-translated at " << row << ", " << row_index[ k ] << " with value " << values[ k ] << ". " << "P = " << P << ", row = " << row << ".\n"; @@ -234,7 +238,7 @@ namespace grb { nonzero.first.second = ActiveDistribution::local_index_to_global( row_index[ k ] - col_off, n, col_pid, P ); nonzero.second = values[ k ]; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "\tInitial pair at " << nonzero.first.first << ", " << nonzero.first.second << " with value " << nonzero.second << ". " << "P = " << P << ", row = " << row << ".\n"; @@ -244,7 +248,7 @@ namespace grb { /** Copy assignment. */ ConstIterator & operator=( const ConstIterator &other ) noexcept { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Matrix (reference) const-iterator copy-assign operator " << "called\n"; #endif @@ -263,7 +267,7 @@ namespace grb { /** Move assignment. */ ConstIterator & operator=( ConstIterator &&other ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Matrix (reference) const-iterator move-assign operator " << "called\n"; #endif @@ -282,7 +286,7 @@ namespace grb { /** Whether two iterators compare equal. */ bool operator==( const ConstIterator &other ) const noexcept { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Compressed_Storage::Const_Iterator operator== called " << "with k ( " << k << ", " << other.k << " ), " << " m ( " << m << ", " << other.m << " )\n"; @@ -308,7 +312,7 @@ namespace grb { /** Whether two iterators do not compare equal. */ bool operator!=( const ConstIterator &other ) const noexcept { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Compressed_Storage::Const_Iterator operator!= called " << "with k ( " << k << ", " << other.k << " ), " << "row ( " << row << ", " << other.row << " ), " @@ -333,7 +337,7 @@ namespace grb { /** Move to the next iterator. */ ConstIterator & operator++() noexcept { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Compressed_Storage::operator++ called\n"; #endif if( row == m ) { @@ -346,7 +350,7 @@ namespace grb { (void) ++row; } if( row < m ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "\tupdated triple, pre-translated at ( " << row << ", " << row_index[ k ] << " ): " << values[ k ] << "\n"; #endif @@ -361,7 +365,7 @@ namespace grb { nonzero.first.second = ActiveDistribution::local_index_to_global( row_index[ k ] - col_off, n, col_pid, P ); nonzero.second = values[ k ]; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "\tupdated triple at ( " << nonzero.first.first << ", " << nonzero.first.second << " ): " << nonzero.second << "\n"; #endif @@ -573,7 +577,7 @@ namespace grb { "internal logic error: InputSIZE must be convertible to SIZE. " "Please submit a bug report" ); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "CompressedStorage::copyFrom (cast) called with range " << start << "--" << end << ". The identity " << (*id) << " will be used.\n"; @@ -680,8 +684,8 @@ namespace grb { * complete if the union of ranges spans 0 to 2nz + m + 1. */ template< - Descriptor descr = descriptors::no_operation, - bool useId = false, + Descriptor descr, + bool useId, typename InputType, typename InputIND, typename InputSIZE > void copyFrom( @@ -691,19 +695,23 @@ namespace grb { const typename std::enable_if< !useId, void >::type * = nullptr ) { static_assert( !std::is_void< InputType >::value, - "InputType must not be void" + "Internal logic error: InputType must not be void. " + "Please submit a bug report." ); static_assert( ( !useId && std::is_convertible< InputType, D >::value ), - "InputType must be convertible to D" + "Internal logic error: InputType must be convertible to D" + "Please submit a bug report." ); static_assert( std::is_convertible< InputIND, IND >::value, - "InputIND must be convertible to IND" + "Internal logic error: InputIND must be convertible to IND" + "Please submit a bug report." ); static_assert( std::is_convertible< InputSIZE, SIZE >::value, - "InputSIZE must be convertible to SIZE" + "Internal logic error: InputSIZE must be convertible to SIZE" + "Please submit a bug report." ); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "CompressedStorage::copyFrom called with range " << start << "--" << end << ". No identity will be used.\n"; #endif @@ -785,7 +793,7 @@ namespace grb { void recordValue( const size_t &pos, const bool row, const fwd_it &it ) { row_index[ pos ] = row ? it.i() : it.j(); values[ pos ] = it.v(); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "\t nonzero at position " << it.i() << " by " << it.j() << " is stored at position " << pos << " has value " << it.v() << ".\n"; #endif @@ -956,7 +964,7 @@ namespace grb { k( 0 ), m( 0 ), n( 0 ), row( 1 ), s( 0 ), P( 1 ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator default constructor (pattern specialisation) " << "called\n"; nonzero.first = 1; @@ -971,7 +979,7 @@ namespace grb { row( other.row ), s( other.s ), P( other.P ), nonzero( other.nonzero ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator copy constructor (pattern specialisation) " << "called\n"; #endif @@ -979,7 +987,7 @@ namespace grb { /** Move constructor. */ ConstIterator( ConstIterator &&other ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator move constructor (pattern specialisation) " << "called\n"; #endif @@ -1004,7 +1012,7 @@ namespace grb { k( 0 ), m( _m ), n( _n ), s( _s ), P( _P ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator constructor (pattern specialisation) called\n"; #endif if( _nz == 0 || _m == 0 || _n == 0 || end ) { @@ -1033,7 +1041,7 @@ namespace grb { /** Copy assignment. */ ConstIterator & operator=( const ConstIterator &other ) noexcept { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator copy-assign operator (pattern specialisation) " << "called\n"; #endif @@ -1051,7 +1059,7 @@ namespace grb { /** Move assignment. */ ConstIterator & operator=( ConstIterator &&other ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "Iterator move-assign operator (pattern specialisation) " << "called\n"; #endif @@ -1288,7 +1296,7 @@ namespace grb { // pattern matrices, but is retained to keep the API // the same as with the non-pattern case. auto k = start; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "CompressedStorage::copyFrom (void) called with range " << start << "--" << end << "\n"; #endif @@ -1346,7 +1354,7 @@ namespace grb { void recordValue( const size_t &pos, const bool row, const fwd_it &it ) { row_index[ pos ] = row ? it.i() : it.j(); // values are ignored for pattern matrices -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "\t nonzero at position " << it.i() << " by " << it.j() << " is stored at position " << pos << ". " << "It records no nonzero value as this is a pattern matrix.\n"; diff --git a/include/graphblas/reference/matrix.hpp b/include/graphblas/reference/matrix.hpp index a560eba92..3d669c5ea 100644 --- a/include/graphblas/reference/matrix.hpp +++ b/include/graphblas/reference/matrix.hpp @@ -2155,14 +2155,16 @@ namespace grb { const size_t start = 0; size_t end = range; #endif - CRS.copyFrom( other.CRS, nz, m, start, end ); + CRS.template copyFrom< descriptors::no_operation, false >( + other.CRS, nz, m, start, end ); range = CCS.copyFromRange( nz, n ); #ifdef _H_GRB_REFERENCE_OMP_MATRIX config::OMP::localRange( start, end, 0, range ); #else end = range; #endif - CCS.copyFrom( other.CCS, nz, n, start, end ); + CCS.template copyFrom< descriptors::no_operation, false >( + other.CCS, nz, n, start, end ); } } From 1e01ed7f3ce5786cd26e88b8e4f80fcf8fd6c2c8 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 31 Jan 2025 12:52:48 +0100 Subject: [PATCH 16/19] Unify copyFrom API, minor code style fix, remove not-well-operating simd vectorisation through OpenMP (of all things that could happen, it surprisingly deadlocked) --- .../reference/compressed_storage.hpp | 45 +++++-------------- include/graphblas/reference/matrix.hpp | 5 ++- 2 files changed, 14 insertions(+), 36 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index fd3204947..c8c501ae3 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -25,11 +25,6 @@ #include //std::memcpy -#if reference == reference_omp - #define _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #include -#endif - #ifdef _DEBUG #define _DEBUG_REFERENCE_COMPRESSED_STORAGE #endif @@ -590,9 +585,6 @@ namespace grb { GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD // type, in which case raw memory copies // are OK -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { if( utils::interpretMatrixMask< descr, InputType >( true, other.getValues(), i ) @@ -617,9 +609,6 @@ namespace grb { GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD // type, in which case raw memory copies // are OK -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { if( utils::interpretMatrixMask< descr, InputType >( true, other.getValues(), i ) @@ -644,9 +633,6 @@ namespace grb { GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD // type, in which case raw memory copies // are OK -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { if( utils::interpretMatrixMask< descr, InputType >( true, other.getValues(), i ) @@ -686,14 +672,22 @@ namespace grb { template< Descriptor descr, bool useId, - typename InputType, typename InputIND, typename InputSIZE + typename InputType, typename InputIND, typename InputSIZE, + typename ValueType > void copyFrom( const Compressed_Storage< InputType, InputIND, InputSIZE > &other, const size_t nz, const size_t m, const size_t start, size_t end, + const ValueType * __restrict__ id, const typename std::enable_if< !useId, void >::type * = nullptr ) { + (void) id; +#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE + std::cout << "CompressedStorage::copyFrom called with range " + << start << "--" << end << ". No identity will be used.\n"; +#endif + // static checks static_assert( !std::is_void< InputType >::value, "Internal logic error: InputType must not be void. " "Please submit a bug report." @@ -711,10 +705,8 @@ namespace grb { "Internal logic error: InputSIZE must be convertible to SIZE" "Please submit a bug report." ); -#ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE - std::cout << "CompressedStorage::copyFrom called with range " - << start << "--" << end << ". No identity will be used.\n"; -#endif + + // do copy size_t k = start; if( k < nz ) { const size_t loop_end = std::min( nz, end ); @@ -722,9 +714,6 @@ namespace grb { GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD // type, in which case raw memory copies // are OK -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { values[ i ] = static_cast< D >( other.values[ i ] ); } @@ -743,9 +732,6 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); } @@ -762,9 +748,6 @@ namespace grb { if( k < m + 1 ) { const size_t loop_end = std::min( m + 1, end ); assert( k <= loop_end ); -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); } @@ -1304,9 +1287,6 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); } @@ -1323,9 +1303,6 @@ namespace grb { if( k < m + 1 ) { const size_t loop_end = std::min( m + 1, end ); assert( k <= loop_end ); -#ifdef _H_GRB_REFERENCE_OMP_COMPRESSED_STORAGE - #pragma omp for simd -#endif for( size_t i = k; i < loop_end; ++i ) { col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); } diff --git a/include/graphblas/reference/matrix.hpp b/include/graphblas/reference/matrix.hpp index 3d669c5ea..c1b3a365d 100644 --- a/include/graphblas/reference/matrix.hpp +++ b/include/graphblas/reference/matrix.hpp @@ -2147,6 +2147,7 @@ namespace grb { #pragma omp parallel #endif { + const char * const dummy = nullptr; size_t range = CRS.copyFromRange( nz, m ); #ifdef _H_GRB_REFERENCE_OMP_MATRIX size_t start, end; @@ -2156,7 +2157,7 @@ namespace grb { size_t end = range; #endif CRS.template copyFrom< descriptors::no_operation, false >( - other.CRS, nz, m, start, end ); + other.CRS, nz, m, start, end, dummy ); range = CCS.copyFromRange( nz, n ); #ifdef _H_GRB_REFERENCE_OMP_MATRIX config::OMP::localRange( start, end, 0, range ); @@ -2164,7 +2165,7 @@ namespace grb { end = range; #endif CCS.template copyFrom< descriptors::no_operation, false >( - other.CCS, nz, n, start, end ); + other.CCS, nz, n, start, end, dummy ); } } From 46f6aebaca236c4267afd5605abdf0483541504a Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 31 Jan 2025 18:39:33 +0100 Subject: [PATCH 17/19] Restore likely-if-possible vectorisation by relying on std::copy_n - going back to how this MR once started in fact:) --- .../reference/compressed_storage.hpp | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index c8c501ae3..e3b41cc4c 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -23,7 +23,8 @@ #ifndef _H_GRB_REFERENCE_COMPRESSED_STORAGE #define _H_GRB_REFERENCE_COMPRESSED_STORAGE -#include //std::memcpy +#include // std::memcpy +#include // std::copy_n #ifdef _DEBUG #define _DEBUG_REFERENCE_COMPRESSED_STORAGE @@ -711,14 +712,7 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - GRB_UTIL_IGNORE_CLASS_MEMACCESS; // by the ALP spec, D can only be a POD - // type, in which case raw memory copies - // are OK - for( size_t i = k; i < loop_end; ++i ) { - values[ i ] = static_cast< D >( other.values[ i ] ); - } - GRB_UTIL_RESTORE_WARNINGS; - + std::copy_n( other.values + k, loop_end - k, values + k ); k = 0; } else { assert( k >= nz ); @@ -732,9 +726,7 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - for( size_t i = k; i < loop_end; ++i ) { - row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); - } + std::copy_n( other.row_index + k, loop_end - k, row_index + k ); k = 0; } else { assert( k >= nz ); @@ -748,9 +740,7 @@ namespace grb { if( k < m + 1 ) { const size_t loop_end = std::min( m + 1, end ); assert( k <= loop_end ); - for( size_t i = k; i < loop_end; ++i ) { - col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); - } + std::copy_n( other.col_start + k, loop_end - k, col_start + k ); #ifndef NDEBUG for( size_t chk = k; chk < loop_end - 1; ++chk ) { assert( other.col_start[ chk ] <= other.col_start[ chk + 1 ] ); @@ -1287,9 +1277,7 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - for( size_t i = k; i < loop_end; ++i ) { - row_index[ i ] = static_cast< IND >( other.row_index[ i ] ); - } + (void) std::copy_n( other.row_index + k, loop_end - k, row_index + k ); k = 0; } else { assert( k >= nz ); @@ -1303,9 +1291,7 @@ namespace grb { if( k < m + 1 ) { const size_t loop_end = std::min( m + 1, end ); assert( k <= loop_end ); - for( size_t i = k; i < loop_end; ++i ) { - col_start[ i ] = static_cast< SIZE >( other.col_start[ i ] ); - } + (void) std::copy_n( other.col_start + k, loop_end - k, col_start + k ); #ifndef NDEBUG for( size_t chk = k; chk < loop_end - 1; ++chk ) { assert( other.col_start[ chk ] <= other.col_start[ chk + 1 ] ); From dca04460e3616d687718d500a5fc74d12d1a592a Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 31 Jan 2025 19:18:58 +0100 Subject: [PATCH 18/19] Final code review --- include/graphblas/bsp1d/io.hpp | 2 +- .../reference/compressed_storage.hpp | 34 ++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/include/graphblas/bsp1d/io.hpp b/include/graphblas/bsp1d/io.hpp index 8f40f1dd9..edada4191 100644 --- a/include/graphblas/bsp1d/io.hpp +++ b/include/graphblas/bsp1d/io.hpp @@ -1723,7 +1723,7 @@ namespace grb { // each thread writes to a different interval of the destination array // give pointers to hint at memmove whenever possible // (StorageType should be trivially copyable) - std::copy_n( + (void) std::copy_n( local_out.data(), num_nnz_local, out.data() + first_nnz_local ); diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index e3b41cc4c..34a2f773d 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -524,8 +524,12 @@ namespace grb { } /** - * Copies coordinates from a given Compressed_Storage, then fills the - * values with the given identity. + * Copies coordinates from a given #Compressed_Storage, \a other. + * + * Optionally, fills the values with a given identity instead of the values + * from \a other; see \a use_id. + * + * Via SFINAE, this variant applies only to \a use_id true. * * Performs no safety checking. Performs no (re-)allocations. * @@ -560,8 +564,7 @@ namespace grb { const ValueType * __restrict__ id, const typename std::enable_if< useId, void >::type * = nullptr ) { - static_assert( - ( useId && std::is_convertible< ValueType, D >::value ), + static_assert( useId && std::is_convertible< ValueType, D >::value, "internal logic error: ValueType must be convertible to D. Please submit " "a bug report" ); @@ -575,8 +578,8 @@ namespace grb { ); #ifdef _DEBUG_REFERENCE_COMPRESSED_STORAGE std::cout << "CompressedStorage::copyFrom (cast) called with range " - << start << "--" << end << ". The identity " << (*id) - << " will be used.\n"; + << start << "--" << end << ". The identity " << (*id) << " will be used " + << ".\n"; #endif assert( start <= end ); size_t k = start; @@ -654,6 +657,11 @@ namespace grb { /** * Copies contents from a given Compressed_Storage. * + * Optionally, fills the values with a given identity instead of the values + * from \a other; see \a use_id. + * + * Via SFINAE, this variant applies only to \a use_id false. + * * Performs no safety checking. Performs no (re-)allocations. * * @param[in] other The container to copy from. @@ -712,7 +720,7 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - std::copy_n( other.values + k, loop_end - k, values + k ); + (void) std::copy_n( other.values + k, loop_end - k, values + k ); k = 0; } else { assert( k >= nz ); @@ -726,7 +734,7 @@ namespace grb { if( k < nz ) { const size_t loop_end = std::min( nz, end ); assert( k <= loop_end ); - std::copy_n( other.row_index + k, loop_end - k, row_index + k ); + (void) std::copy_n( other.row_index + k, loop_end - k, row_index + k ); k = 0; } else { assert( k >= nz ); @@ -740,7 +748,7 @@ namespace grb { if( k < m + 1 ) { const size_t loop_end = std::min( m + 1, end ); assert( k <= loop_end ); - std::copy_n( other.col_start + k, loop_end - k, col_start + k ); + (void) std::copy_n( other.col_start + k, loop_end - k, col_start + k ); #ifndef NDEBUG for( size_t chk = k; chk < loop_end - 1; ++chk ) { assert( other.col_start[ chk ] <= other.col_start[ chk + 1 ] ); @@ -1254,15 +1262,15 @@ namespace grb { * \internal copyFrom specialisation for pattern matrices. */ template< - Descriptor = descriptors::no_operation, - bool unusedValue = false, + Descriptor, + bool unusedValue, typename InputType, typename InputIND, typename InputSIZE, - typename UnusedType = std::nullptr_t + typename UnusedType > void copyFrom( const Compressed_Storage< InputType, InputIND, InputSIZE > &other, const size_t nz, const size_t m, const size_t start, size_t end, - const UnusedType * __restrict__ = nullptr + const UnusedType * __restrict__ ) { (void) unusedValue; // the unusedValue template is meaningless in the case of From 1e863da74ae473eaa83875d185f5b6b2a7f3d7ea Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 31 Jan 2025 19:19:56 +0100 Subject: [PATCH 19/19] Remove unnecessary check for use_id --- include/graphblas/reference/compressed_storage.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/graphblas/reference/compressed_storage.hpp b/include/graphblas/reference/compressed_storage.hpp index 34a2f773d..95b67a903 100644 --- a/include/graphblas/reference/compressed_storage.hpp +++ b/include/graphblas/reference/compressed_storage.hpp @@ -564,9 +564,9 @@ namespace grb { const ValueType * __restrict__ id, const typename std::enable_if< useId, void >::type * = nullptr ) { - static_assert( useId && std::is_convertible< ValueType, D >::value, - "internal logic error: ValueType must be convertible to D. Please submit " - "a bug report" + static_assert( std::is_convertible< ValueType, D >::value, + "internal logic error: ValueType must be convertible to D. " + "Please submit a bug report" ); static_assert( std::is_convertible< InputIND, IND >::value, "internal logic error: InputIND must be convertible to IND. "