Integrate Modified AC-SpGEMM / GALATIC #26

richardlett · 2021-06-23T01:14:44Z

GALATIC repo isn't public yet, draft PR. Don't accept

Notes:

C++14 now required
CUDA 11 now required
--expt-relaxed-constexpr now required. We can remove this with a hack, if need be.
- (we depend on std::limit , I think, to find max of an integral type parameter - technically not legal CUDA, but it's a constexpr, so it's evaluated at compile time anyway)

512 thread AC-SpGEMM/GALATIC version differs in most parameters due to large shared memory because # of threads, otherwise stuck with defaults. May have issues with merging -- needs testing.

I haven't looked at larger matrices (mostly curious about merging with 512 threads) or did perf testing
currently only threads configurable parameter
more optimized for smaller sized types, don't have contingency for larger sized types. Not sure if I'm the bottle neck ATM

ctcyang

Nice work! I was able to successfully build and everything works. Tested that this does get the correct result for the testcase in Issue #20. I also tested compilation time compared to before this PR. Small increase from 5min to 6min, which isn't something to worry about.

Mainly have nitpicks (minor issues) below.

ctcyang · 2021-06-23T09:00:55Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "ext/moderngpu"]
 	path = ext/moderngpu
 	url = https://ctcyang@github.com/ctcyang/moderngpu.git


nitpick: Do you mind changing both of these submodule URLs to a consistent format such as https://github.com/ctcyang/moderngpu.git and https://github.com/richardlett/GALATIC.git?

changed them both to ssh as that will be more universal until repo is public (automatically uses your ssh-key)

ctcyang · 2021-06-23T09:19:28Z

graphblas/backend/cuda/spgemm.hpp

@@ -1,14 +1,24 @@
 #ifndef GRAPHBLAS_BACKEND_CUDA_SPGEMM_HPP_
 #define GRAPHBLAS_BACKEND_CUDA_SPGEMM_HPP_

+
+#include "../../../ext/GALATIC/include/dCSR.cuh"


I'm not a big fan of relative path includes that are dependent on a file being in the right folder. Could you add the galatic directory "GALATIC/include" to the CMakeLists.txt includes?

Something else that would be nice to have is a single file helper file in GALATIC that includes all helper files part of your public interface. Then we can just have a one-liner #include "galatic.cuh". This will let you iterate on GALATIC really quickly, for example add new include + source files or change naming of files in your repo, without breaking GALATIC's dependencies (e.g. graphblast).

Some examples for how to do that are "cub.cuh", ModernGPU's "moderngpu.cuh" and "graphblast.hpp".

Thanks for pointing this out! You can tell I don't have the most experience with C++ :p

I feel that way about myself too, but most important thing is to code lots and learn something new every day =P

ctcyang · 2021-06-23T09:19:30Z

graphblas/backend/cuda/spgemm.hpp

@@ -1,14 +1,24 @@
 #ifndef GRAPHBLAS_BACKEND_CUDA_SPGEMM_HPP_
 #define GRAPHBLAS_BACKEND_CUDA_SPGEMM_HPP_

+


nitpick: stylistic thing - I like having only single-spaced lines between lines with code, so could you get rid of all the newline spacing? 1-line gaps are fine, so don't worry about those.

ctcyang · 2021-06-23T09:19:34Z

graphblas/backend/cuda/operations.hpp

+             &B->sparse_, desc));
+        else {
+            if (s_mode != GrB_GALATIC) {
+                std::cout << R"(Unknown mode (Options are: "cusspare2" and "galatic"; defaulting to galatic)" << std::endl;


nitpick: fix typo cusspare2 -> cusparse2

ctcyang · 2021-06-23T09:19:43Z

graphblas/backend/cuda/spgemm.hpp

 #include "graphblas/backend/cuda/sparse_matrix.hpp"

+


nit: get rid of empty line

ctcyang · 2021-06-23T09:19:45Z

graphblas/backend/cuda/spgemm.hpp

 #include <cuda.h>
 #include <cusparse.h>

 #include <iostream>
 #include <vector>

+


nit: get rid of empty lines

ctcyang · 2021-06-23T09:19:50Z

graphblas/backend/cuda/spgemm.hpp

+  matrixToGalatic(A, leftInputMatrixGPU);
+  matrixToGalatic(B, rightInputMatrixGPU);
+
+


nit: get rid of extra lines and the rest in this file.

ctcyang

Thanks for making the formatting changes! Added a few minor comments, but in general looks good to me!

ctcyang · 2021-06-30T06:53:07Z

CMakeLists.txt

 set( mgpu_SRC_FILES "ext/moderngpu/src/mgpucontext.cu" "ext/moderngpu/src/mgpuutil.cpp")
 set( CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/../bin )
 #set( CUDA_CURAND_LIBRARY "$ENV{CUDA_HOME}/lib64/libcurand.so" )
 #set( CUDA_CUBLAS_LIBRARY "$ENV{CUDA_HOME}/lib64/libcublas.so" )
 set( CUDA_CUSPARSE_LIBRARY "$ENV{CUDA_HOME}/lib64/libcusparse.so" )
-#FILE( GLOB_RECURSE PROJ_SOURCES graphblas/*.cu ../graphblas/*.cpp )
+#FILE( G     LOB_RECURSE PROJ_SOURCES graphblas/*.cu ../graphblas/*.cpp )


nit: extra tab here

ctcyang · 2021-06-30T06:53:22Z

graphblas/backend/cuda/operations.hpp

@@ -52,7 +52,7 @@ Info mxm(Matrix<c>*       C,
             &B->sparse_, desc));
        else {
            if (s_mode != GrB_GALATIC) {
-                std::cout << R"(Unknown mode (Options are: "cusspare2" and "galatic"; defaulting to galatic)" << std::endl;
+                std::cout << R"(Unknown mode (Options are: "cuspare2" and "galatic"; defaulting to galatic)" << std::endl;


nit: cusparse2

ctcyang · 2021-06-30T07:00:53Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "ext/moderngpu"]
 	path = ext/moderngpu
-	url = https://ctcyang@github.com/ctcyang/moderngpu.git
+	url = git@github.com:ctcyang/moderngpu.git


nit: Kind of a personal preference thing, but could you change these to use HTTPS verification rather than SSH? I usually don't have ssh-key installed, so I typically use HTTPS.

I've tried with HTTPS and for people with access to GALATIC private repo, then so long as that they enter my Github login + pw, it lets them pull in the submodule.

ctcyang · 2021-06-30T07:01:00Z

graphblas/backend/cuda/spgemm.hpp

-#include "../../../ext/GALATIC/include/dCSR.cuh"
-#include "../../../ext/GALATIC/include/SemiRingInterface.h"
-#include "../../../ext/GALATIC/source/device/Multiply.cuh"
+#include <GALATIC/GALATICMinimumIncludes.cuh>


Do you mind changing this to #include "GALATICMinimumIncludes.cuh"? According to the Google C++ style guide, typically nonsystem headers use the quotation marks instead of angled brackets.

That way, the PROJ_INCLUDES can be set( PROJ_INCLUDES "./" "ext/moderngpu/include" "ext/GALATIC"), so it's easier for someone trying to install graphblast to debug for example when PROJ_INCLUDES can't find the GALATIC folder.

Whoops, bad inference made me think that was for files that were resolved based on relative path to current file -I!

richardlett · 2021-07-02T21:31:49Z

Sorry for delay, that should do it!

ctcyang

LGTM aside from a few nitpicks. I'm happy to merge this when GALATIC becomes public, so give me a heads up when!

ctcyang · 2021-07-04T05:27:19Z

CMakeLists.txt

 #set(CUDA_NVCC_FLAGS ${CUDA_NVCC_FLAGS};-fpermissive;-arch=sm_35;-lineinfo;-Xptxas=-v;-dlcm=ca;-maxrregcount=64)
 #set(CUDA_NVCC_FLAGS ${CUDA_NVCC_FLAGS};-gencode arch=compute_20,code=sm_21)
 # needed for cudamalloc
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
-set(CMAKE_CXX_FLAGS "-fpermissive -g -m64 -std=c++11" )
+set(CMAKE_CXX_FLAGS "-fpermissive -g  -std=c++14" )


nit: one space instead of two between -g and -std=c++14

ctcyang · 2021-07-04T05:27:42Z

Makefile

@@ -5,7 +5,7 @@ include common.mk
 #-------------------------------------------------------------------------------

 # Includes
-INC += -I$(MGPU_DIR) -I$(CUB_DIR) -I$(BOOST_DIR) -I$(GRB_DIR)
+INC += -I$(MGPU_DIR)  -I$(BOOST_DIR) -I$(GRB_DIR)


nit: one space instead of two between MGPU_DIR and BOOST_DIR

ctcyang · 2021-07-04T05:28:24Z

graphblas/backend/cuda/spgemm.hpp

@@ -108,6 +110,182 @@ Info spgemmMasked(SparseMatrix<c>*       C,
  C->csc_initialized_ = false;
  return GrB_SUCCESS;
 }
+// Shallow copy graphblast sparsematrix -> Galatic dCSR format


nit: please add a newline between following template function and above function.

cyborgshead · 2022-08-13T04:38:09Z

@ctcyang
@richardlett made Galactic public and I think that it will be a huge movement for gunrock's graphblas to support not only default matrix format but sparse with csr that will open the road to huge graph processing

Will be great if examples will be also updated
I hope that that development will be continued because there is increasing interest in graph processing, especially with graphblas, and gunrock may push this movement with leading GPU support

richardlett added 10 commits June 21, 2021 19:09

Integration WIP, it builds

b9bd4bc

formatting

179666b

Better integration, select threads/spgemm

5080cc3

slight cleanup

4cb56c7

some formatting and minor adjustments

ca71c72

edit readme

b9e075c

removed submodule cub

2c7bdb9

removed build directory

99c74ba

update submodule

04138ae

renamed first and second to something that won't conflict with thrust

50fa929

ctcyang reviewed Jun 23, 2021

View reviewed changes

ctcyang linked an issue Jun 23, 2021 that may be closed by this pull request

Incorrect MinPlus semiring output #20

Open

fixed formatting issues

ce4a855

ctcyang reviewed Jun 30, 2021

View reviewed changes

formatting/spelling fixes

dac2a22

ctcyang reviewed Jul 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Modified AC-SpGEMM / GALATIC #26

Integrate Modified AC-SpGEMM / GALATIC #26

richardlett commented Jun 23, 2021 •

edited

Loading

ctcyang left a comment •

edited

Loading

ctcyang Jun 23, 2021 •

edited

Loading

richardlett Jun 29, 2021

ctcyang Jun 23, 2021 •

edited

Loading

richardlett Jun 23, 2021

ctcyang Jun 25, 2021 •

edited

Loading

ctcyang Jun 23, 2021 •

edited

Loading

ctcyang Jun 23, 2021

ctcyang Jun 23, 2021

ctcyang Jun 23, 2021

ctcyang Jun 23, 2021

ctcyang left a comment

ctcyang Jun 30, 2021

ctcyang Jun 30, 2021

richardlett Jul 2, 2021

ctcyang Jun 30, 2021 •

edited

Loading

ctcyang Jun 30, 2021

richardlett Jul 2, 2021

richardlett commented Jul 2, 2021

ctcyang left a comment

ctcyang Jul 4, 2021

ctcyang Jul 4, 2021

ctcyang Jul 4, 2021

cyborgshead commented Aug 13, 2022

		@@ -1,14 +1,24 @@
		#ifndef GRAPHBLAS_BACKEND_CUDA_SPGEMM_HPP_
		#define GRAPHBLAS_BACKEND_CUDA_SPGEMM_HPP_

		matrixToGalatic(A, leftInputMatrixGPU);
		matrixToGalatic(B, rightInputMatrixGPU);

Integrate Modified AC-SpGEMM / GALATIC #26

Are you sure you want to change the base?

Integrate Modified AC-SpGEMM / GALATIC #26

Conversation

richardlett commented Jun 23, 2021 • edited Loading

ctcyang left a comment • edited Loading

Choose a reason for hiding this comment

ctcyang Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctcyang Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctcyang Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

ctcyang Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctcyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctcyang Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlett commented Jul 2, 2021

ctcyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyborgshead commented Aug 13, 2022

richardlett commented Jun 23, 2021 •

edited

Loading

ctcyang left a comment •

edited

Loading

ctcyang Jun 23, 2021 •

edited

Loading

ctcyang Jun 23, 2021 •

edited

Loading

ctcyang Jun 25, 2021 •

edited

Loading

ctcyang Jun 23, 2021 •

edited

Loading

ctcyang Jun 30, 2021 •

edited

Loading