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

[WIP] CUDA error checking/debugging #94

Merged
merged 24 commits into from
Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .clang-format
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ BasedOnStyle: Google
---
Language: Cpp
Cpp11BracedListStyle: true
Standard: Cpp11
Standard: c++11
DerivePointerAlignment: false
PointerAlignment: Right
---
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake)
include(cpplint)
include(glog)
include(googletest)
include(gtest_utils)
include(pybind11)
include(cub)

Expand Down
64 changes: 64 additions & 0 deletions cmake/gtest_utils.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
enable_testing()

function(k2_add_gtest)
cmake_parse_arguments(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This cmake code is very confusing, for me and I'm sure the majority of readers . Can you please add some kind of comment that's aimed at someone who doesn't really understand cmake? That explains what implications it has for usage, in terms of what flags you can pass or what behaviors it causes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, maybe I can change another way to reuse the gtest function. But for now, as to add a CMake module doesn't break things, I would like to put it here for convenience.

ARGS # prefix of output variables
"EXCLUDE_FROM_ALL" # list of names of the boolean arguments (only defined ones will be true)
"" # list of names of mono-valued arguments
"SOURCES;DEPENDS" # list of names of multi-valued arguments (output variables are lists)
${ARGN} # arguments of the function to parse, here we take the all original ones
) # remaining unparsed arguments can be found in ARGS_UNPARSED_ARGUMENTS

set(dependencies ${ARGS_DEPENDS})

list(LENGTH ARGS_UNPARSED_ARGUMENTS other_args_size)
if(other_args_size EQUAL 1)
set(name ${ARGS_UNPARSED_ARGUMENTS})
else()
message(FATAL_ERROR "too many or not enough unnamed arguments")
endif()

if(ARGS_SOURCES)
set(sources ${ARGS_SOURCES})
else()
message(FATAL_ERROR "you must specify at least one source for the test")
endif()

if(ARGS_EXCLUDE_FROM_ALL)
set(EXCLUDE_FROM_ALL "EXCLUDE_FROM_ALL")
endif()

string(REGEX REPLACE "^.*\\." "" tgt ${name})
add_executable(${tgt} ${sources} ${EXCLUDE_FROM_ALL})

target_link_libraries(${tgt} PUBLIC ${dependencies} gtest)
target_link_libraries(${tgt} PRIVATE gtest_main)

add_test(NAME ${name} COMMAND $<TARGET_FILE:${tgt}>)

set(${CMAKE_PROJECT_NAME}_all_tests "${${CMAKE_PROJECT_NAME}_all_tests};${tgt}" CACHE STRING "" FORCE)
endfunction()

# directly give the suite name source files,
function(k2_add_gtests test_set_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the following template
https://github.com/danpovey/k2/blob/2a6ba24b4a7845bab757fbf7044b0d6253bcb2d8/k2/csrc/CMakeLists.txt#L29-L41
not good enough that you want to make your own?

I find this cmake script hard to read and understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it's not quite readable. My motivation was to make this function reusable (and could keep gtest-compile out of make process, e.g. only compile and run if make test.). As simply put the above codes into a CMake module, it still requires the sources and dep-libs. If you will, please help make this function reusable?

cmake_parse_arguments(
ARGS # prefix of output variables
"EXCLUDE_FROM_ALL" # list of names of the boolean arguments (only defined ones will be true)
"" # list of names of mono-valued arguments
"SOURCES;DEPENDS" # each source share the same args in this api, and each's a sub-gtest
${ARGN} # arguments of the function to parse, here we take the all original ones
) # remaining unparsed arguments can be found in ARGS_UNPARSED_ARGUMENTS

list(LENGTH ARGS_UNPARSED_ARGUMENTS other_args_size)
if(NOT other_args_size EQUAL 0)
message(FATAL_ERROR "too many or not enough unnamed arguments")
endif()

foreach(source ${ARGS_SOURCES})
string(REGEX REPLACE "\\..*$" "" name ${source})
k2_add_gtest("${test_set_name}.${name}"
SOURCES ${source}
DEPENDS ${ARGS_DEPENDS}
)
endforeach()
endfunction()
2 changes: 2 additions & 0 deletions k2/csrc/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
include_directories(${CMAKE_SOURCE_DIR})

# please sort the source files alphabetically
add_library(fsa
arcsort.cc
Expand Down
35 changes: 10 additions & 25 deletions k2/csrc/cuda/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,28 +1,13 @@
add_library(context context.cu)
target_include_directories(context PUBLIC ${CMAKE_SOURCE_DIR})
target_link_libraries(context PUBLIC cub)
target_link_libraries(context PUBLIC glog)
set(CUDA_SOURCE
context.cu
)

function(k2_add_cuda_test name)
add_executable(${name} "${name}.cu")
target_link_libraries(${name}
PRIVATE
context
gtest
gtest_main
)
add_test(NAME "Test.Cuda.${name}"
COMMAND
$<TARGET_FILE:${name}>
)
endfunction()
add_library(k2_cuda ${CUDA_SOURCE})
target_include_directories(k2_cuda PUBLIC ${CMAKE_SOURCE_DIR})
target_link_libraries(k2_cuda PUBLIC cub)
target_link_libraries(k2_cuda PUBLIC glog)

# please sort the source files alphabetically
set(cuda_tests
ops_test
utils_test
k2_add_gtests(Test.Cuda
SOURCES debug_test.cu utils_test.cu #ops_test.cu
Copy link
Collaborator

Choose a reason for hiding this comment

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

please put the source filenames in a variable, like the above CUDA_SOURCE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this module, cmake_parse_arguments is used, which accepts these into a set. So we don't need to do it out of func.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some time later, this line will contain tens of cuda source filenames, which harms readability.

DEPENDS k2_cuda cub glog
)

foreach(name IN LISTS cuda_tests)
k2_add_cuda_test(${name})
endforeach()
32 changes: 32 additions & 0 deletions k2/csrc/cuda/arch.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// k2/csrc/cuda/arch.h

// Copyright (c) 2020 Xiaomi Corporation ( authors: Meixu Song )

// See ../../LICENSE for clarification regarding multiple authors
#ifndef K2_CSRC_CUDA_ARCH_H_
#define K2_CSRC_CUDA_ARCH_H_

namespace k2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to use a namespace here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can remove arch.h...


/**
* @def K2_PTX_ARCH
*
* @brief
* K2_PTX_ARCH get the target PTX version through the active compiler.
*
* @details
* - for host, it's 0.
* - for device, it's `__CUDA_ARCH__`, which indicates the
* compute compatibility, should >= 200.
*/
#ifndef K2_PTX_ARCH
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have macro guards around each definition of a macro. Can't you just use an include guard around the entire header? Wouldn't that be simpler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The include guard is already there. The intension is to avoid conflict with following redefine the same name macro outer the header. Though it rarely happens, sometimes could, e.g. conflict with K2_PARANOID_ASSERT from kaldi-error.h, LOG() from glog.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Macros that start with K2_ don't need include guards as they're specific to this project.

#ifndef __CUDA_ARCH__
#define K2_PTX_ARCH 0
#else
#define K2_PTX_ARCH __CUDA_ARCH__
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not to use __CUDA_ARCH__ directly in k2?

#endif
#endif

} // namespace k2

#endif // K2_CSRC_CUDA_ARCH_H_
1 change: 1 addition & 0 deletions k2/csrc/cuda/context.cu
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <cstdlib>

#include "k2/csrc/cuda/context.h"
#include "k2/csrc/cuda/debug.h"
#include "k2/csrc/cuda/error.h"

static constexpr size_t kAlignment = 64;
Expand Down
Loading