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 2 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
76 changes: 76 additions & 0 deletions cmake/gtest_utils.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
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
"NO_MAIN;NO_GMOCK;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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think gtest, gmock, gtest_main and gmock_main all can be passed via
DEPENDS and there is no need to use separate options for NO_MAIN, NO_GMOCK,
which can simplify this function significantly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

${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()

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

if(ARGS_NO_GMOCK)
target_link_libraries(${tgt} PUBLIC ${dependencies} gtest)
Copy link
Collaborator

@csukuangfj csukuangfj Aug 30, 2020

Choose a reason for hiding this comment

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

why is it PUBLIC for gtest and gmock, but PRIVATE for the subsequent gtest_main and gmock_main?

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 PRIVATE keep the most depends on the target. It's suggested to compile executables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_main would lead to an executable.

else()
target_link_libraries(${tgt} PUBLIC ${dependencies} gtest gmock)
endif()

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

if(ARGS_NO_MAIN)
elseif(ARGS_NO_GMOCK)
target_link_libraries(${tgt} PRIVATE gtest_main)
else()
target_link_libraries(${tgt} PRIVATE gmock_main)
endif()

add_test(NAME ${name} COMMAND $<TARGET_FILE:${tgt}>)
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})
get_filename_component(name ${source} NAME_WE)
k2_add_gtest("${test_set_name}.${name}"
NO_GMOCK
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
37 changes: 12 additions & 25 deletions k2/csrc/cuda/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,28 +1,15 @@
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)
include_directories(${CMAKE_SOURCE_DIR})
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to add this statement. It can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


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()

# please sort the source files alphabetically
set(cuda_tests
ops_test
utils_test
set(CUDA_SOURCE
context.cu
)

foreach(name IN LISTS cuda_tests)
k2_add_cuda_test(${name})
endforeach()
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)

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
)
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