From 6abbe45068c96c7bda7ad7f412b58ee483e4fd13 Mon Sep 17 00:00:00 2001 From: Giovanni Gaio <48856010+GiovaGa@users.noreply.github.com> Date: Thu, 30 Oct 2025 10:39:30 +0100 Subject: [PATCH 1/8] Added test reproducing bug --- tests/unit/dot.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/dot.cpp b/tests/unit/dot.cpp index 4279109ce..958e62dd4 100644 --- a/tests/unit/dot.cpp +++ b/tests/unit/dot.cpp @@ -160,6 +160,38 @@ void grb_program( const size_t &n, grb::RC &rc ) { return; } + // test 5, init + rc = rc ? rc : grb::clear( x ); + rc = rc ? rc : grb::clear( y ); + assert( rc == grb::SUCCESS ); + int true_dot = 0; + alpha = 0; + + for(size_t i = n; i > 0 ; --i ){ + rc = rc ? rc : grb::setElement( y, i-1, i-1 ); + } + for( size_t i : {2,3,5,7,13,17,19,23,29} ){ + if( i >= n ) break; + rc = rc ? rc : grb::setElement( x, 1, i ); + true_dot += i; + + } + assert( rc == grb::SUCCESS ); + + // test 4, exec + rc = grb::dot( alpha, x, y, intRing ); + if( rc != SUCCESS ) { + std::cerr << "\t test 5 (non constant-value vectors) dot FAILED\n"; + return; + } + + // test 4, check + if( alpha != true_dot ) { + std::cerr << "\t test 5 (non constant-value vectors) unexpected value " + << alpha << ", expected " << true_dot << ".\n"; + rc = FAILED; + return; + } } int main( int argc, char ** argv ) { From a5e29557aac9489cca6d390db60c8d5c9e13528f Mon Sep 17 00:00:00 2001 From: Giovanni Gaio <48856010+GiovaGa@users.noreply.github.com> Date: Fri, 31 Oct 2025 09:13:49 +0100 Subject: [PATCH 2/8] fixup! Added test reproducing bug --- tests/unit/dot.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/unit/dot.cpp b/tests/unit/dot.cpp index 958e62dd4..f658837a6 100644 --- a/tests/unit/dot.cpp +++ b/tests/unit/dot.cpp @@ -167,25 +167,24 @@ void grb_program( const size_t &n, grb::RC &rc ) { int true_dot = 0; alpha = 0; - for(size_t i = n; i > 0 ; --i ){ - rc = rc ? rc : grb::setElement( y, i-1, i-1 ); + for(size_t i = 0; i < n ; ++i ){ + rc = rc ? rc : grb::setElement( y, i, i ); } for( size_t i : {2,3,5,7,13,17,19,23,29} ){ if( i >= n ) break; rc = rc ? rc : grb::setElement( x, 1, i ); true_dot += i; - } assert( rc == grb::SUCCESS ); - // test 4, exec + // test 5, exec rc = grb::dot( alpha, x, y, intRing ); if( rc != SUCCESS ) { std::cerr << "\t test 5 (non constant-value vectors) dot FAILED\n"; return; } - // test 4, check + // test 5, check if( alpha != true_dot ) { std::cerr << "\t test 5 (non constant-value vectors) unexpected value " << alpha << ", expected " << true_dot << ".\n"; From 99d3cca75d50fda6f33f01339d9056c17c53414f Mon Sep 17 00:00:00 2001 From: Giovanni Gaio <48856010+GiovaGa@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:03:58 +0100 Subject: [PATCH 3/8] Fixing bug (seemingly) --- include/graphblas/nonblocking/blas1.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/graphblas/nonblocking/blas1.hpp b/include/graphblas/nonblocking/blas1.hpp index c4853c7b2..5ff3b04bd 100644 --- a/include/graphblas/nonblocking/blas1.hpp +++ b/include/graphblas/nonblocking/blas1.hpp @@ -10504,9 +10504,9 @@ namespace grb { for( ; i < local_nz; ++i ) { typename AddMonoid::D3 temp = addMonoid.template getIdentity< typename AddMonoid::D3 >(); - const size_t index = ( already_dense_input_y ? i : local_y.index( i ) ) + + const size_t index = ( already_dense_input_x ? i : local_x.index( i ) ) + lower_bound; - if( already_dense_input_x || local_x.assigned( index - lower_bound ) ) { + if( already_dense_input_y || local_y.assigned( index - lower_bound ) ) { apply( temp, a[ index ], b[ index ], anyOp ); foldr( temp, thread_local_output, addMonoid.getOperator() ); } From 8fede66c5f61d2722d1e795c4b1f439b0470ce2b Mon Sep 17 00:00:00 2001 From: Giovanni Gaio <48856010+GiovaGa@users.noreply.github.com> Date: Tue, 25 Nov 2025 09:55:34 +0100 Subject: [PATCH 4/8] Further fixes --- include/graphblas/nonblocking/blas1.hpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/graphblas/nonblocking/blas1.hpp b/include/graphblas/nonblocking/blas1.hpp index 5ff3b04bd..648e999fb 100644 --- a/include/graphblas/nonblocking/blas1.hpp +++ b/include/graphblas/nonblocking/blas1.hpp @@ -10461,7 +10461,7 @@ namespace grb { for( size_t k = 0; k < AnyOp::blocksize; ++k, ++i ) { if( mask[ k ] ) { xx[ k ] = static_cast< typename AnyOp::D1 >( - a[ ( already_dense_input_y ? i : local_y.index( i ) ) + lower_bound ] ); + a[ ( already_dense_input_x ? i : local_x.index( i ) ) + lower_bound ] ); yy[ k ] = static_cast< typename AnyOp::D2 >( b[ ( already_dense_input_y ? i : local_y.index( i ) ) + lower_bound ] ); } @@ -10504,10 +10504,11 @@ namespace grb { for( ; i < local_nz; ++i ) { typename AddMonoid::D3 temp = addMonoid.template getIdentity< typename AddMonoid::D3 >(); - const size_t index = ( already_dense_input_x ? i : local_x.index( i ) ) + + const size_t index_a = ( already_dense_input_x ? i : local_x.index( i ) ) + lower_bound; - if( already_dense_input_y || local_y.assigned( index - lower_bound ) ) { - apply( temp, a[ index ], b[ index ], anyOp ); + if( already_dense_input_y || local_y.assigned( i ) ) { + const size_t index_b = ( already_dense_input_y ? i : local_y.index( i ) ) + lower_bound; + apply( temp, a[ index_a ], b[ index_b ], anyOp ); foldr( temp, thread_local_output, addMonoid.getOperator() ); } } From 9bd9f93f557b0233ba3aab9e41396f8412f62faf Mon Sep 17 00:00:00 2001 From: Giovanni Gaio <48856010+GiovaGa@users.noreply.github.com> Date: Tue, 25 Nov 2025 13:39:13 +0100 Subject: [PATCH 5/8] Partial revert "Further fixes" --- include/graphblas/nonblocking/blas1.hpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/graphblas/nonblocking/blas1.hpp b/include/graphblas/nonblocking/blas1.hpp index 648e999fb..6d70af825 100644 --- a/include/graphblas/nonblocking/blas1.hpp +++ b/include/graphblas/nonblocking/blas1.hpp @@ -10504,11 +10504,10 @@ namespace grb { for( ; i < local_nz; ++i ) { typename AddMonoid::D3 temp = addMonoid.template getIdentity< typename AddMonoid::D3 >(); - const size_t index_a = ( already_dense_input_x ? i : local_x.index( i ) ) + + const size_t index = ( already_dense_input_x ? i : local_x.index( i ) ) + lower_bound; - if( already_dense_input_y || local_y.assigned( i ) ) { - const size_t index_b = ( already_dense_input_y ? i : local_y.index( i ) ) + lower_bound; - apply( temp, a[ index_a ], b[ index_b ], anyOp ); + if( already_dense_input_y || local_y.assigned( index - lower_bound ) ) { + apply( temp, a[ index ], b[ index ], anyOp ); foldr( temp, thread_local_output, addMonoid.getOperator() ); } } From 8463a86d4a9f4a34e555568a208f600a7be82f67 Mon Sep 17 00:00:00 2001 From: Albert-Jan Yzelman Date: Thu, 18 Dec 2025 18:34:02 +0100 Subject: [PATCH 6/8] The implementation looked off to me (the same indices should be read from the left and right inputs). I extended the unit tests to try trip up the new implementation, but I appear to have not been able to construct a test that does-- oddly enough. Nevertheless, will keep the extended unit test --- tests/unit/dot.cpp | 61 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 6 deletions(-) diff --git a/tests/unit/dot.cpp b/tests/unit/dot.cpp index f658837a6..63a6f1fb8 100644 --- a/tests/unit/dot.cpp +++ b/tests/unit/dot.cpp @@ -27,6 +27,7 @@ void grb_program( const size_t &n, grb::RC &rc ) { // repeatedly used containers grb::Vector< bool > even_mask( n ); + grb::Vector< bool > odd_mask( n ); grb::Vector< size_t > temp( n ); grb::Vector< double > left( n ); grb::Vector< double > right( n ); @@ -42,7 +43,14 @@ void grb_program( const size_t &n, grb::RC &rc ) { }, temp ); rc = rc ? rc : grb::set( even_mask, temp, true ); if( rc != grb::SUCCESS ) { - std::cerr << "\t initialisation of mask FAILED\n"; + std::cerr << "\t initialisation of even mask FAILED\n"; + return; + } + + // create odd mask + rc = grb::set< grb::descriptors::invert_mask >( odd_mask, temp, true ); + if( rc != grb::SUCCESS ) { + std::cerr << "\t initialisation of odd mask FAILED\n"; return; } @@ -167,9 +175,7 @@ void grb_program( const size_t &n, grb::RC &rc ) { int true_dot = 0; alpha = 0; - for(size_t i = 0; i < n ; ++i ){ - rc = rc ? rc : grb::setElement( y, i, i ); - } + rc = rc ? rc : grb::set< grb::descriptors::use_index >( y, 0 ); for( size_t i : {2,3,5,7,13,17,19,23,29} ){ if( i >= n ) break; rc = rc ? rc : grb::setElement( x, 1, i ); @@ -180,17 +186,60 @@ void grb_program( const size_t &n, grb::RC &rc ) { // test 5, exec rc = grb::dot( alpha, x, y, intRing ); if( rc != SUCCESS ) { - std::cerr << "\t test 5 (non constant-value vectors) dot FAILED\n"; + std::cerr << "\t test 5 (sparse non constant-value vectors) dot FAILED\n"; return; } // test 5, check if( alpha != true_dot ) { - std::cerr << "\t test 5 (non constant-value vectors) unexpected value " + std::cerr << "\t test 5 (sparse non constant-value vectors) unexpected value " << alpha << ", expected " << true_dot << ".\n"; rc = FAILED; return; } + + // test 6, init + // Note: there is a bug with std::swap for nonblocking. Need to raise an issue(!) + //std::swap( x, y ); + // meanwhile, work around: + rc = grb::set( y, x ); + rc = rc ? rc : grb::set< grb::descriptors::use_index >( x, 0 ); + alpha = 0; + + // test 6, exec + rc = rc ? rc : grb::dot( alpha, x, y, intRing ); + if( rc != SUCCESS ) { + std::cerr << "\t test 6 (swapped version of test 5) dot FAILED\n"; + return; + } + + // test 6, check + if( alpha != true_dot ) { + std::cerr << "\t test 6 (swapped version of test 5) unexpected value " + << alpha << ", expected " << true_dot << ".\n"; + rc = FAILED; + return; + } + + // test 7, init + rc = grb::set< grb::descriptors::use_index >( x, odd_mask, 0 ); + rc = rc ? rc : grb::set< grb::descriptors::use_index >( y, even_mask, 0 ); + alpha = 0; + + // test 7, exec + rc = rc ? rc : grb::dot( alpha, x, y, intRing ); + if( rc != SUCCESS ) { + std::cerr << "\t test 7 (sparse dot no overlap) FAILED\n"; + return; + } + + // test 7, check + if( alpha != 0 ) { + std::cerr << "\t test 7 (sparse dot no overlap) unexpected value " + << alpha << ", expected 0.\n"; + rc = FAILED; + return; + } } int main( int argc, char ** argv ) { From c7a64d81debb66f33e7aecc6a2b232269cdfb8a2 Mon Sep 17 00:00:00 2001 From: Albert-Jan Yzelman Date: Thu, 18 Dec 2025 18:35:21 +0100 Subject: [PATCH 7/8] Revert one of the previous fixes -- we should always iterate over the nonzeroes of x, since that is the vector with guaranteed the fewest number of nonzeroes. Instead, the error was in the computation of the mask -- it used y instead of x. Special note that the other fix WAS correct-- so there were two bugs in total. Also a minor code style fix --- include/graphblas/nonblocking/blas1.hpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/include/graphblas/nonblocking/blas1.hpp b/include/graphblas/nonblocking/blas1.hpp index 6d70af825..1ac780da7 100644 --- a/include/graphblas/nonblocking/blas1.hpp +++ b/include/graphblas/nonblocking/blas1.hpp @@ -10398,6 +10398,9 @@ namespace grb { // internal namespace for implementation of grb::dot namespace internal { + /** + * Invariant: x should have fewer nonzeroes than y + */ template< Descriptor descr, #ifdef GRB_BOOLEAN_DISPATCHER @@ -10432,6 +10435,7 @@ namespace grb { #else (void) upper_bound; #endif + assert( local_x.nonzeroes() <= local_y.nonzeroes() ); // get raw alias const InputType1 * __restrict__ a = internal::getRaw( x ); @@ -10450,8 +10454,7 @@ namespace grb { // prepare registers for( size_t k = 0; k < AnyOp::blocksize; ++k, ++i ) { - mask[ k ] = already_dense_input_x || - local_x.assigned( already_dense_input_y ? i : local_y.index( i ) ); + mask[ k ] = already_dense_input_x || local_y.assigned(local_x.index( i )); } // rewind @@ -10463,7 +10466,7 @@ namespace grb { xx[ k ] = static_cast< typename AnyOp::D1 >( a[ ( already_dense_input_x ? i : local_x.index( i ) ) + lower_bound ] ); yy[ k ] = static_cast< typename AnyOp::D2 >( - b[ ( already_dense_input_y ? i : local_y.index( i ) ) + lower_bound ] ); + b[ ( already_dense_input_x ? i : local_x.index( i ) ) + lower_bound ] ); } } @@ -10658,7 +10661,9 @@ namespace grb { already_dense_input_y, already_dense_input_x, array_reduced[ thread_id ], lower_bound, upper_bound, - local_y, local_x, x, y, local_y_nz, + local_y, local_x, + x, y, + local_y_nz, addMonoid, anyOp ); } From 824fed84dbbed686ca8c9e2ec31e23abaf5d2070 Mon Sep 17 00:00:00 2001 From: Albert-Jan Yzelman Date: Tue, 23 Dec 2025 03:18:02 +0100 Subject: [PATCH 8/8] Raised issue 408, removed comments --- tests/unit/dot.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/unit/dot.cpp b/tests/unit/dot.cpp index 63a6f1fb8..326b4fe88 100644 --- a/tests/unit/dot.cpp +++ b/tests/unit/dot.cpp @@ -199,9 +199,6 @@ void grb_program( const size_t &n, grb::RC &rc ) { } // test 6, init - // Note: there is a bug with std::swap for nonblocking. Need to raise an issue(!) - //std::swap( x, y ); - // meanwhile, work around: rc = grb::set( y, x ); rc = rc ? rc : grb::set< grb::descriptors::use_index >( x, 0 ); alpha = 0;