Use libcu++ interfaces. #3297

mzient · 2021-08-30T15:26:02Z

Signed-off-by: Michał Zientkiewicz mzient@gmail.com

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

It switches from using RMM to libcu++.
It implements the simple resources from RMM which we had previously used.
It changes the behavior of the linter so that headers without an extension are interpreted as C++

Additional information

Affected modules and functionalities:

Memory management
Linter

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-2268

dali-automaton · 2021-08-30T16:09:02Z

CI MESSAGE: [2874299]: BUILD STARTED

dali-automaton · 2021-08-30T16:56:37Z

CI MESSAGE: [2874299]: BUILD FAILED

dali-automaton · 2021-08-31T08:23:08Z

CI MESSAGE: [2879811]: BUILD STARTED

szalpal · 2021-08-31T09:52:34Z

CMakeLists.txt

@@ -254,6 +254,8 @@ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-command-line-argument")
    # Ignore warnings coming from cutlass
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --system-header-prefix=cutlass/")
+    # Enable SFINAE with ellipsis in device code
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Xclang -fcuda-allow-variadic-functions")


This whole if(DALI_CLANG_ONLY) section is about temporarily turning off warnings, that should be addressed (per @klecki comment above). Since this option is not turning off any warning, but enabling a feature, maybe it should be marked appropriately?

I agree, no issue with turning this on, probably split it into warning section and useful feature section.

I can move the comment and run build stage only again if you REALLY insist.

Done - I had to fix basename extraction in linter, so another build is in order.

szalpal · 2021-08-31T10:48:47Z

dali/core/mm/async_pool_test.cu

@@ -163,12 +162,14 @@ void AsyncPoolTest(Pool &pool, vector<block> &blocks, Mutex &mtx, CUDAStream &st
      CUDA_CALL(cudaMemsetAsync(ptr, fill, size, stream));
      {
        std::lock_guard<Mutex> guard(mtx);
+        (void)guard;  // for dummy mutexes


Could you explain this? I'm not sure I understand this line correctly

Sometimes (in the tests) the mutex is a dummy object where the lock/unlock functions are empty and the compiler is smart enough to see that the construction/destruction has no effect and emits a warning.

szalpal · 2021-08-31T10:49:00Z

.gitmodules

 [submodule "third_party/thrust"]
 	path = third_party/thrust
 	url = https://github.com/NVIDIA/thrust
+[submodule "third_party/libcudacxx"]
+	path = third_party/libcudacxx
+	url = https://github.com/mzient/libcudacxx.git


Is linking from your private fork on purpose?

It's not been upstreamed yet, so yes, it's necessary for the time being.
See: NVIDIA/libcudacxx#158

szalpal · 2021-08-31T10:53:21Z

third_party/cpplint.py

@@ -4480,7 +4480,8 @@ def _ClassifyInclude(fileinfo, include, is_system):
            or include.endswith(".hpp") \
            or include.endswith(".hxx") \
            or include.endswith(".H") \
-            or include.endswith(".hh")
+            or include.endswith(".hh") \
+            or '.' not in include[include.rfind('/')]  # no extension


Are we sure we want this? I recall you saying, that that's in order to have #include <cuda/memory_resource> recognized as C++. However, we are not really running linter for third party headers, so I don't feel it's necessary to run it for <cuda/memory_resource>. On the other hand, this may lead to strange linter behaviour in future.

@banasraf , what's your opinion about it?

If I understand correctly, it's not about running linter on a third party header, but about ordering of includes. Linter requires some certain order of includes, and this change is to correctly qualify the cuda/memory_resource as C++ include.

^^^ exactly that.

dali-automaton · 2021-08-31T12:17:43Z

CI MESSAGE: [2879811]: BUILD PASSED

dali-automaton · 2021-08-31T12:43:47Z

CI MESSAGE: [2881094]: BUILD STARTED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2021-08-31T15:14:19Z

CI MESSAGE: [2882032]: BUILD STARTED

dali-automaton · 2021-08-31T16:05:44Z

CI MESSAGE: [2881094]: BUILD PASSED

dali-automaton · 2021-08-31T17:12:09Z

CI MESSAGE: [2882032]: BUILD PASSED

NVIDIA deleted a comment from dali-automaton Aug 30, 2021

mzient force-pushed the UseLibCudaCxx branch from 3ba3a12 to 62df976 Compare August 31, 2021 08:21

klecki assigned szalpal and banasraf Aug 31, 2021

banasraf approved these changes Aug 31, 2021

View reviewed changes

szalpal reviewed Aug 31, 2021

View reviewed changes

szalpal approved these changes Aug 31, 2021

View reviewed changes

mzient and others added 5 commits August 31, 2021 17:12

Use libcu++ interfaces.

fb5478f

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Add missing include in numpy_reader_gpu_op.

2233ed1

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Make linter interpret headers without an extension as C++.

a666b1b

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Fix clang-specific errors.

0a9f071

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Review issues.

5e82c9b

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the UseLibCudaCxx branch from a409a28 to 5e82c9b Compare August 31, 2021 15:12

mzient merged commit f722661 into NVIDIA:main Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use libcu++ interfaces. #3297

Use libcu++ interfaces. #3297

mzient commented Aug 30, 2021 •

edited

Loading

dali-automaton commented Aug 30, 2021

dali-automaton commented Aug 30, 2021

dali-automaton commented Aug 31, 2021

szalpal Aug 31, 2021

klecki Aug 31, 2021

mzient Aug 31, 2021 •

edited

Loading

mzient Aug 31, 2021

szalpal Aug 31, 2021

mzient Aug 31, 2021

szalpal Aug 31, 2021

mzient Aug 31, 2021 •

edited

Loading

szalpal Aug 31, 2021

banasraf Aug 31, 2021

mzient Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

Use libcu++ interfaces. #3297

Use libcu++ interfaces. #3297

Conversation

mzient commented Aug 30, 2021 • edited Loading

Description

What happened in this PR

Additional information

Affected modules and functionalities:

Checklist

Tests

Documentation

DALI team only

Requirements

dali-automaton commented Aug 30, 2021

dali-automaton commented Aug 30, 2021

dali-automaton commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Aug 31, 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

mzient Aug 31, 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

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

dali-automaton commented Aug 31, 2021

mzient commented Aug 30, 2021 •

edited

Loading

mzient Aug 31, 2021 •

edited

Loading

mzient Aug 31, 2021 •

edited

Loading