Sparse #1
Sparse #1
Conversation
include/doGemm.hh
Outdated
| iterations_, gpuTime_every, | ||
| calcGflops(calcFlops(M, N, K), iterations_, gpuTime_every)); | ||
| } | ||
|
|
||
| void callSparseKernels(std::ofstream& csvFile, const int N, const double sparsity) { |
There was a problem hiding this comment.
Rather than have the sparce kernels also in doGemm, I think having a seperate do<KERNEL> for each kernel type would make it a bit easier to do kernel specific updates.
Creating a doSpMM.hh or doSpGemm.hh I think would be beneficial, copying a similar structure to this file and then adding the appropriate calls in main.cc
include/helpers.hh
Outdated
| @@ -28,15 +28,16 @@ std::ofstream initCSVFile(const std::string filename) { | |||
| * Function does not close the file. */ | |||
| void writeLineToCsv(std::ofstream& file, const std::string device, | |||
| const std::string kernel, const int M, const int N, | |||
| const int K, const double totalProbSize, const int iters, | |||
| const int K, const double sparsity, const double totalProbSize, const int iters, | |||
There was a problem hiding this comment.
Could it be worth us having a writeLineToCSV type function for each kernel? There will probably be more instances where there will be kernel specific parameters we want to print out.
DefaultCPU/spGemm.hh
Outdated
| using spGemm<T>::spGemm; | ||
| using spGemm<T>::n_; | ||
|
|
||
| /** Initialise the required data structures. */ |
There was a problem hiding this comment.
Given the above todo, may be worth adding to this comment what type of sparce matrix is being initialised currently
DefaultCPU/spGemm.hh
Outdated
| } | ||
|
|
||
| private: | ||
| bool rMat(std::vector<T>& M, int n, int x1, int x2, int y1, int y2, |
There was a problem hiding this comment.
Please add a brief comment above this function explaining what it does
DefaultCPU/spGemm.hh
Outdated
| int xMidPoint = x1 + (int)((x2 - x1) / 2); | ||
| int yMidPoint = y1 + (int)((y2 - y1) / 2); | ||
|
|
||
| // ToDo - consider if need to check for non-square matrices |
There was a problem hiding this comment.
You could add a static_assert to the top of this function to ensure the matrix is always square?
DefaultCPU/spGemm.hh
Outdated
| // float newA = a; | ||
| // float newB = b; | ||
| // float newC = c; |
There was a problem hiding this comment.
Remove un-used variables
DefaultCPU/spGemm.hh
Outdated
| * A return value is required to ensure that the compiler does not optimise | ||
| * away this function. */ |
There was a problem hiding this comment.
This comment is from my implementation, but this last sentence isn't true anymore. Please remove
DefaultCPU/spGemm.hh
Outdated
| void printGraph(std::vector<T> M, int n) { | ||
| std::cout << "____Printing Matrix____" << std::endl; | ||
| for (int i = 0; i < n; i++) { | ||
| for (int j = 0; j < n; j++) { | ||
| std::cout << M[(i * n) + j] << " "; | ||
| } | ||
| std::cout << std::endl; | ||
| } | ||
| } |
DefaultCPU/spGemm.hh
Outdated
| while (!rMat(A_, n, 0, n-1, 0, n-1, | ||
| 0.45, 0.22, 0.22, | ||
| gen, dist)) { | ||
| gen.seed(std::chrono::system_clock::now() | ||
| .time_since_epoch().count()); | ||
| } | ||
| while (!rMat(B_, n, 0, n-1, 0, n-1, | ||
| 0.45, 0.22, 0.22, | ||
| gen, dist)) {} |
There was a problem hiding this comment.
Could you add a comment to each while explaining what is being done?
include/CPU/spGemm.hh
Outdated
| /** Matrix size -- matrix will be nxn */ | ||
| int n_ = 0; |
There was a problem hiding this comment.
If all matricies are always nxn then may be worth updating the comment to " -- all matrices will be nxn" to be very explicit?
.idea/.gitignore
Outdated
There was a problem hiding this comment.
.idea/ needs adding to .gitignore
DefaultCPU/spGemm.hh
Outdated
| #include <vector> | ||
| #include <chrono> | ||
|
|
||
| #include "../include/CPU/spGemm.hh" |
There was a problem hiding this comment.
I think we should follow general BLAS naming convensions and rename spGemm to spmm universally
DefaultCPU/spGemm.hh
Outdated
| } | ||
|
|
||
| private: | ||
| bool rMat(std::vector<T>* M, int n, int x1, int x2, int y1, int y2, |
There was a problem hiding this comment.
Add short comment above function detailing what it is doing
src/main.cc
Outdated
| std::cout << " --dense Run only the dense matrix kernels " | ||
| "(cannot be run in combination with --sparse)" << std::endl; | ||
| std::cout << " -s --sparse Run only the sparse matrix kernels " | ||
| "(cannot be run in combination with --dense)" << std::endl; | ||
| std::cout << std::endl; | ||
| exit(0); | ||
| } else if (!strcmp(argv[i], "--sparse") || !strcmp(argv[i], "-s")) { | ||
| dense = false; | ||
| } else if (!strcmp(argv[i], "--dense")) { | ||
| sparse = false; |
There was a problem hiding this comment.
As discussed offline, I think having a sinlge --kernels= argument followed by a comma seperated list would be a cleaner option and avoid scenarios above where --dense cannot have -d also assigned to it.
In use, the argument would look like --kernel=gemm,spmm,gemv for example
There was a problem hiding this comment.
Also if --kernel is not given, we should default to running all kernels
src/main.cc
Outdated
| std::cout << "\tRunning dense kernels: " << denseStr << std::endl; | ||
| std::cout << "\tRunning sparse kernels: " << sparseStr << std::endl; |
There was a problem hiding this comment.
I like this idea of printing which kernels have been enabled. With the suggested change to the input argument, perhaps printing Running dense kernels: gemm gemv etc would mean that the print out list doesn't keep growing with each new kernel type added? Plus it gives the user 100% clarity on what exact dense / sparce kernels are being run
ABenC377
force-pushed
the
sparse
branch
2 times, most recently
from
57054b2
to
74fe223
Compare
Adding in Sparse CPU Kernel. Currently just runs a square matrix with a sparsity of 0.99 for each problem size.