Conversation
First attempt at a basic GitHub CI which tests bootstrap, basic build, and installation. If successful, it will then also run the smoke test target. Smoke tests that require datasets are skipped.
…ses the Hermitian symmetry type. TODO: implement conjugation during Hermitian read-out.
… is not causing the current failure
…alue in MatrixFileProperties, herewith fixed. This commit also adds in some more verbose debug tracing of the parser.
…first buffer element was not copied
…put-and-interpret-hermitian-stored-in-upper-triangle This MR modifies the parser to read MatrixMarket files that hold complex-valued nonzeroes, and furthermore handles Hermitian matrix files correctly. Complex values are read into the C++ `std::complex` type, which also provides the required complex conjugation operation. This MR paves the way for smoke tests of existing iterative solvers using complex-valued system matrices. This unit test will be added via later MR(s). This MR also updates the coding style of the affected files.
Resolves internal issue 399: demo complex CG using std::complex This MR includes all necessary changes to the parser and the CG algorithm in order to solve complex-valued linear systems. It includes a smoke test on a randomly generated system, included with the test suite, which verifies the CG algorithm versus known output. The algorithmic changes mostly related to complex conjugation during dot products, necessary since while a descriptor for transposition (of matrices) exist, ALP/GraphBLAS does not include a descriptor indicating a Hermitian operator that furthermore should apply to one of multiple input containers. CG now instead implements conjugation using `grb::eWiseLambda`, which requires the use of an additional temporary vector. Both overheads in terms of computation and data movement are *not* incurred for non-complex CG via the use of the newly included `grb::utils::is_complex` type trait.
Update the README.md, in particular also linking to GitHub project location and issue tracker. Reviewed all text and ensured that all up to and including the quick starts read as concisely as possible. Closes GitHub issue #4 .
Fix typo -- apologies
Update the table of (additional) contents. Made first subsection title of the integration section mesh better with that of the second subsection.
Fix two broken links
#11) This MR introduces a proof-of-concept where ALP/GraphBLAS is used to generate a (subset) of the standard blas_sparse.h and spblas.h SparseBLAS APIs. The former is the SparseBLAS standard as proposed by NIST and the BLAS forum. The latter is a de-facto standard that is based on a pre-standard version of the NIST SparseBLAS. With this merge, CMake will install the spblas and blas_sparse headers in the `include/transition` directory, and will install `libsparseblas` and `libsparseblas_omp` into `lib/sequential`; that is, we generate both a sequential as well as a shared-memory parallel version of both SparseBLAS APIs. This is dubbed a "transition path" since it allows the use of software as-is currently implemented based on a standard API without having to re-write (parts of) the user code to follow the ALP syntax -- even though the latter may be simple to do. For the SparseBLAS API this does not come at any noticeable overhead for the currently implemented functions, while for the SpBLAS API some overhead is incurred for the matrix--vector and matrix--matrix multiplication routines. This is due to the transparent matrix type in spblas, versus an opaque one in the standard SparseBLAS. This does *not* mean that the performance is totally on par with ALP/GraphBLAS, even when using the standard API; for example, the optimisations performed by the nonblocking backend of ALP/GraphBLAS cannot be applied in the transition path (unless a JIT approach is followed). The development history of this proof-of-concept may be viewed in the branch associated to Github PR #11 or the branch associated to internal issue 411 on Gitee. This MR also: - the reference matrix output iterator could enter an infinite loop and go out-of-bounds, herewith fixed; - the reference grb::eWiseApply ([T1]<-T2<-T3), monoid variant would EXECUTE instead of RESIZE when the latter was asked for, herewith fixed; - the reference allocator may, for large allocations, print an ill-formatted string, herewith fixed; - adds a SparseBLAS extension for opaque vector types, adds a sparse matrix--sparse vector multiplication using that type, adds a sparse matrix--sparse matrix multiplication for the SparseBLAS standard (not for spblas, as one was already commonly defined there), and adds sparse vector and matrix read-out routines; - ensured compilation with LLVM Clang (though this remains not "officially" supported at this stage); - fixes various code style issues.
…es class does not match the default one of the reference or reference_omp backend.
…irm density when dense descriptor is given to foldl (vector to scalar) (#19) Aristeidis detected that when giving a dense descriptor and a sparse vector to `grb::foldl` (vector to scalar reduction), that the resulting code will complete the requested computation. It was instead intended, like with other primitives, that `ILLEGAL` is returned instead since the programmer promises a certain property that does not hold in practice. This is conform the intent that the dense descriptor 1. prevents the generation of code that handles sparse cases, and 2. achieves (minor) performance benefits as it can skip some of the associated checks for sparsity. This MR ensures the intended behaviour is what occurs. It also: - reduces some code duplication throughout the code base; - removes grb::Phase as an argument to reduction to a scalar, since the output is not a container; - revises and updates the documentation regarding `grb::{foldl,foldr}` (vector to scalar variants); - introduces an input-masked `grb::foldr` variant. Input masking existed for the `grb::foldl` in order to support HPCG, but introducing a similar input mask for the foldr appeared to have been overlooked, herewith fixed; - introduces tests for the input-masked foldr; - introduces tests for when ILLEGAL should be returned; - introduces tests for when MISMATCH should be returned; - simplifies the reduction of a vector to scalar unit test by removing the (duplicate) performance test it had embedded; - increase unit tests coverage with non-trivial masks and `descriptors::invert_mask`. As always, also included are some minor code style and typo fixes across the files this MR touches. (The commit history of this MR is retained in branch 441-missing-run-time-checks-to-confirm-density-when-dense-descriptor-is-given.)
…e all on the last row: fixing this
… if not, the destructor of a default-initialized matrix triggers some assertions; if this occurs, e.g., when an exception is thrown the assertion triggers and the original error does not surface
buildMatrixUnique split for iterator types buildMatrixUnique has two version call, one for forward iterators and one for random access iterators random access iterators can be fully used in omp
cum sum omp function moved to random access iterator version of buildMatrixUnique the last loop prallelized but using omp critical omp parallel counting sort buildMatrixUnique split for iterator types buildMatrixUnique has two version call, one for forward iterators and one for random access iterators random access iterators can be fully used in omp cumulative sum parallelized cum sum omp function moved to random access iterator version of buildMatrixUnique omp parallel counting sort
logic to call right builder implementation based on SFINAE
adding test for rectangular matrix adding iterator for diagonal matrix and sorting output of matrix iterators before comparison using specific storage format for nonzeroes based on std::pair's
…ception; allowing sequential matrix build for sequential-only backends
…ckend, to build in parallel also locally wiring out result of buildMatrixUnique implementations adding test harness to unittests factoring out noo-zero computation as a preparation for parallelization
implementing dispatching logic based on iterator type dispatching buildMatrixUnique call based solely on iterator tag; better handling error conditions; better tracing and logging facilities improving test harness with more tests and more thorough conditions using static scheduling for input iterator ingestion factorizing parallel/sequential iterator implementation to a single call and comparing matrices built from different kinds of iterators in addition to different IOmode's using operator++ for iterator rather than operator+= count_sort_omp now returns success/error code; getting row/columns ID via specialized template rather than lambda; minor cleanups
checking buffer availability and code-style cleanups thread-local prefix sum to build the final rows array in the CSR freeing buffer
added 8 commits
August 7, 2022 10:00
…#36) In the `reference_omp` backend, as part of the previous merge into develop (#35), Alberto detected that increasing the chunk size of statically-scheduled parallel-for loops could increase the performance noticeably. The intent of the previous chunk size was to prevent false sharing effects by considering a unit of loop iterands considered by OpenMP to be at least of some size `k`, where `k` is the cache line size. The idea was that then no threads will cause more than O(1) false sharing effects, even if the loop touches single-byte elements-- or indeed zero such effects in the case all memory regions are aligned. However, despite its name, the OpenMP chunked static schedule introduces a run-time component by assigning the chunks to threads in a round-robin fashion-- aside from run-time overhead, this may also cause more page misses than otherwise would occur. This MR removes all parallel-fors with static chunked schedules and replaces it with code that only achieves the above intended effect-- a loop is chunked into units of `k` iterands, and then a static schedule is applied on a loop of size `ceil(n/k)`, where `n` is the actual loop size. This is done via the pre-existing (in ALP) `config::OMP::localRange` that was designed to achieve exactly the intended behaviour from within OpenMP parallel sections such as in `vxm_generic`. This MR also replaces several uses of `#pragma omp for` from within such sections by this pattern. It also adds some more sanity checking to calls to `grb::zip` (to matrix outputs) and fixes an older version of `grb::zip` still present in the Banshee backend. As always, it also includes some code style fixes across the files that this MR touches. This resolves internal issue #479.
In eWiseMulAdd's polyalgorithm, the dense variant implements the case where the call generates dense output, but supports to cases regarding the output vector: 1) on entry the output vector was dense, or 2) on entry the output vector was totally empty. The switch on these two cases was handled by `z_assigned` Boolean template argument, but in fact when true was assigning directly to the value arrays of the output vector, which is valid in the case where z initially was empty. Hence `assign_z` (or something like `z_was_empty`) is a more correct name. This MR changes `z_assigned` into `assign_z` and joins it with some minor code style fixes. The dispatcher furthermore determines the value of `assign_z` based partially on duplicate code. While if `sparse` is set `z_assign` is not used, this MR clarifies this by setting `assign_z` to true only if also `!sparse`. It also prevents materialising identical sparse variants based on switching `z_assign` there. Finally, the MR also adds clarifying code documentations. It corresponds to internal issue #487. Thanks to Aristeidis for reporting.
The documentation of the dense descriptor was incorrect and could do with clarifying corner cases. This MR adds such documentation, and also moves an exposed internal include from the header file to the corresponding `descriptors.cpp`. It also clarifies the `toString' output of the dense descriptor, and applies minor code style fixes. This MR corresponds to, and addresses, internal issue #485. Thanks to Aristeidis for reporting.
… nonzero variable (#39) Anders reported that copy-assignment of matrix output iterators was not properly implemented -- the local nonzero cache was not updated. The same was true for the copy-constructor, move-constructor, and move-assignment. This MR: 1. fixes all those issues, and 2. hardens the matrixIterator unit test to test all four of {move,copy}x{assignment,construction} for all combination of pattern- and non-pattern matrices it already was testing for. Additionally, 3. The assignment operators of the pattern-matrix output iterator specialisation did not return a reference to itself, with this MR also fixed; 4. the matrix output iterator itself now also adheres to the ALP matrix iterator API-- i.e., it defines an i, j, and v member function that return the row, column, and value of a nonzero, respectively. It also now and accordingly defines the `RowIndexType`, `ColumnIndexType`, and `ValueType` typedefs; 5. the SynchronizedNonzeroIterator was missing the ALP `ValueType` typedef for pattern matrices, herewith fixed. This resolves internal issue #462.
Anders reported that the `tests/unit/dense_spmv.cpp` failed verification for `ecology2.mtx`. The reason was that the test relies on `grb::utils::equals` to compare numbers vs. an absolute error bound, however, the utility function is written for relative error checking. This MR clarifies the documentation of `grb::utils::equals', and hardens its API somewhat to prevent misuse.
Summarising, this MR:
- implements an absolute error check for `dense_spmv.cpp` instead of relying on `grb::utils::equals`;
- bugfix: BiCGstab also relied on `grb::utils::equals` while it should not;
- bugfix `grb::utils::equals`: the given relative bound was interpreted twice as loose;
- bugfix `grb::utils::equals`: overflow of the normalisation factor was handled questionably;
- testing: unit test added for `grb::utils::equals` to prevent regressions of its implementation;
- code improvement: number of epsilons to `grb::utils::equals` must be integer to help prevent misuse of `grb::utils::equals`;
- tests that did not pass integer epsilons to `grb::utils::equals` are now fixed;
- improved documentation of `grb::utils::equals` to help prevent its misuse.
All unit tests touched were modernised:
- grb::{init,finalize}->grb::Launcher,
- do not use printf/fprintf,
- standardised use of stderr/stdout and test result printing.
Additional loosely related items:
- BiCGstab test for orthogonality (As vs. s) compared the wrong scalar output (was rho, should be temp), now fixed;
- fixed typo in `smoketests.sh` reporting on two related tests.
The MR includes code style fixes through most files it touches.
This MR reviews all uses of the dynamic schedule and replaces it with a static schedule (through `grb::config::OMP::localRange`) in case the loop is determined to be regular. In all irregular loop cases, documentation has been added with rationale as per the schedule choice, as well as for cases where the choice is not clear-cut.
This MR also modifies the following unrelated behaviour:
- eWise{Map,Lambda} no longer serialise lambda invocations when `_DEBUG` is defined. Rationale: if this is desired, the user can add this to the lambda itself-- by default, it is desirable that lambdas run concurrently in order to diagnose any data race issues, for example.
As always, this MR includes some code style fixes throughout all files touched.
… fault (#45) Aristeidis and Anders reported an issue where combinations of `clear`, `setElement`, and usage of the `PinnedVector` could segfault for the `reference` and `reference_omp` backends. This MR first and foremost introduces a unit test for the `PinnedVector` that operates on vectors of `double`s, vectors of `std::pair`s, and tests it using both the SEQUENTIAL and PARALLEL I/O modes. This added unit test found or confirmed the following issues, which this MR also fixes: - `grb::set` (BSP1D) contains code to handle the `use_index` descriptor, but this code should only pass by the compiler if `size_t`s are convertible into the vector element type; - `grb::PinnedVector` constructor (reference, reference_omp, and BSP1D) should pin the stack, not the bitmask; - `grb::clear` (BSP1D) did not undo the `became_dense` flag, potentially resulting in a distributed vector that is both cleared and became dense; - `grb::PinnedVector` constructor (BSP1D) now throws expections instead of silently ignoring errors during construction. Also minor performance enhancements, code style fixes, better debug tracing, code documentation improvements, and a fixed typo. This MR closes internal issue #422.
Merging the first release candidate for v0.6.0 into develop. Changes since v0.5 are noted in the Changelog. This MR also checks and updates the compiler warning suppressions, and includes small code style fixes across the files it touches. At this point, the code has been thoroughly checked using the following software stack combinations: - OS: Fedora 21, CentOS Stream release 8, Ubuntu 20.04 LTS; - GCC: 8.5.0, 9.3.1, and 9.4.0; - MPI: MPICH 3.3.2, OpenMPI 4.0.2, and OpenMPI 4.1.2; - Linux kernels: 4.18.0 and 5.0.4.
commented
Aug 14, 2022
Member
Author
|
Waiting internal CI |
commented
Aug 14, 2022
Member
Author
|
Internal CI OK, merging to master |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merging the first release candidate for v0.6.0 into master. Changes since v0.5 are noted in the Changelog.
At this point, the code has been thoroughly checked using the following software stack combinations:
Crossref GitHub issue #47