Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Modified AC-SpGEMM / GALATIC #26

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

richardlett
Copy link

@richardlett richardlett commented Jun 23, 2021

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

Copy link
Collaborator

@ctcyang ctcyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1,6 +1,6 @@
[submodule "ext/moderngpu"]
path = ext/moderngpu
url = https://[email protected]/ctcyang/moderngpu.git
Copy link
Collaborator

@ctcyang ctcyang Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


#include "../../../ext/GALATIC/include/dCSR.cuh"
Copy link
Collaborator

@ctcyang ctcyang Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@ctcyang ctcyang Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


Copy link
Collaborator

@ctcyang ctcyang Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: fix typo cusspare2 -> cusparse2

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


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: get rid of empty line

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

#include <iostream>
#include <vector>


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: get rid of empty lines

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


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@ctcyang ctcyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CMakeLists.txt Outdated
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 )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra tab here

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: cusparse2

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 wow

.gitmodules Outdated
@@ -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
Copy link
Collaborator

@ctcyang ctcyang Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#include "../../../ext/GALATIC/include/dCSR.cuh"
#include "../../../ext/GALATIC/include/SemiRingInterface.h"
#include "../../../ext/GALATIC/source/device/Multiply.cuh"
#include <GALATIC/GALATICMinimumIncludes.cuh>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@richardlett
Copy link
Author

Sorry for delay, that should do it!

Copy link
Collaborator

@ctcyang ctcyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#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" )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: one space instead of two between MGPU_DIR and BOOST_DIR

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cyborgshead
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect MinPlus semiring output
3 participants