Add Laplacian GPU operator #3644

stiepan · 2022-01-25T20:45:17Z

Category:

New feature (non-breaking change which adds functionality)

Description:

Adds GPU Laplacian operator. Splits instantiations of operator implementation with different In/Out template parameters to allow for parallel compilation (trick akin to the one in gaussian blur).
Moves a class that computes laplacian windows to the separate header in kernels dir.

Additional information:

Affected modules and functionalities:

Laplacian CPU

Key points relevant for the review:

Don't be discouraged with the number of added lines, 300 of them is boilerplate for splitting the instantiation of op impl in separate files.

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: LAPL.01 - LAPL.17

JIRA TASK: DALI-2438

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-01-25T20:45:32Z

!build

dali-automaton · 2022-01-25T20:51:39Z

CI MESSAGE: [3826306]: BUILD STARTED

dali-automaton · 2022-01-25T22:44:00Z

CI MESSAGE: [3826306]: BUILD PASSED

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-01-26T15:30:31Z

dali/kernels/imgproc/convolution/laplacian_windows.h

@@ -0,0 +1,125 @@
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


moved from laplacian_params.h

It's not 1:1 copy though :P

In the hindsight - it is the worst kind of moving the code around.

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-01-26T15:49:58Z

!build

dali-automaton · 2022-01-26T15:56:10Z

CI MESSAGE: [3832078]: BUILD STARTED

stiepan · 2022-01-26T16:03:35Z

dali/operators/image/convolution/laplacian_params.h

-constexpr static const int maxWindowSize = 23;
-
-template <typename T>
-class LaplacianWindows {


moved to laplacian_windows.h

dali-automaton · 2022-01-26T17:20:48Z

CI MESSAGE: [3832078]: BUILD PASSED

dali-automaton · 2022-01-27T16:09:43Z

CI MESSAGE: [3840048]: BUILD STARTED

klecki

Looks ok, few comments.
Please check the test running time and the compilation time.

klecki · 2022-01-28T15:07:54Z

dali/operators/image/convolution/laplacian.h

@@ -29,13 +29,20 @@ namespace dali {
 #define LAPLACIAN_CPU_SUPPORTED_TYPES \
  (uint8_t, int8_t, uint16_t, int16_t, uint32_t, int32_t, uint64_t, int64_t, float16, float)

+// TODO(klecki): float16 support - it's not easily compatible with float window,
+// need to introduce some cast in between and expose it in the kernels
+// #define LAPLACIAN_GPU_SUPPORTED_TYPES (float)


There appears to be an old comment here.

I purposely copied that part as it seems to apply here as well as the op is based on the gpu convolution. I thought it would be easier to navigate, but maybe it is unnecessary or irrelevant?

Just the // #define LAPLACIAN_GPU_SUPPORTED_TYPES (float) part

Oh, obviously. I did not notice that even when pointed to. Thanks :D

klecki · 2022-01-28T15:10:25Z

dali/operators/image/convolution/laplacian_gpu.h

+op_impl_uptr GetLaplacianGpuImpl(const OpSpec& spec,
+                                                            const DimDesc& dim_desc) {


Nitpick: weird formatting.

klecki · 2022-01-28T15:14:50Z

dali/operators/image/convolution/laplacian_gpu.h

+    BOOL_SWITCH(dim_desc.is_channel_last(), HasChannels, (
+      BOOL_SWITCH(dim_desc.is_sequence(), IsSeq, (
+        using LaplacianImpl = LaplacianOpGpu<Out, In, Axes, HasChannels, IsSeq>;
+        return std::make_unique<LaplacianImpl>(spec, dim_desc);


From my recollection using make_unique in code that already has layers of templates resulted in super slow compilation times.
That's why for gaussian and arithm ops I used

std::unique_ptr<OpImplBase<GPUBackend>> result; SWITCH( ...) ( result.reset(new OpGpu<...>(...)); );

Can you check it?

Okay, I remember wondering why the reset method. I'll check it.

klecki · 2022-01-28T15:45:45Z

dali/operators/image/convolution/laplacian_gpu.h

+ * to allow for parallel compilation of underlying kernels.
+ */
+template <typename Out, typename In>
+op_impl_uptr GetLaplacianGpuImpl(const OpSpec& spec,


As you are replicating a pattern from Gaussian Blur, you might want to take a look into the reasoning in: #3472 to have a GSG-style pass-by-pointer here. I'm just posting this, but I'm not sure I agree with the reasoning.

klecki · 2022-01-28T15:58:21Z

dali/operators/image/convolution/laplacian_gpu.h

+
+    std::array<bool, axes> has_smoothing = uniform_array<axes>(false);
+    for (int sample_idx = 0; sample_idx < nsamples; sample_idx++) {
+      const auto& window_sizes = args.GetWindowSizes(sample_idx);


The LaplacianArgs prohibits from smoothing one sample and not smoothing other sample or is it allowed?
Can we have some weird case where smoothing window would have a 1D size for one sample and be empty for other? Or is it still 1D with {0} size?

There are no empty windows and the smallest window size is 1 (that corresponds to a window [1]). If for given partial derivative all samples don't require smoothing the whole list of windows is empty. If some samples require smoothing and some don't, those that don't will be convolved with [1].

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-01-31T16:49:32Z

!build

dali-automaton · 2022-01-31T16:55:26Z

CI MESSAGE: [3862472]: BUILD STARTED

dali-automaton · 2022-01-31T18:14:54Z

CI MESSAGE: [3862472]: BUILD PASSED

* Add Laplacian GPU operator * Move LaplacianWindows to kernels * Add slow attr to some of Laplacian Python tests Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan added 3 commits January 25, 2022 14:41

Add Laplacian GPU operator

da617d2

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Move LaplacianWindows to kernels

1863f24

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Clearer LaplacianWindows tests

2ec1e75

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan added 2 commits January 26, 2022 12:22

Replace exp2 with exp2f calls

6328ca3

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Split compilation into separate units

c83234d

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan commented Jan 26, 2022

View reviewed changes

stiepan marked this pull request as ready for review January 26, 2022 15:41

Remove unnecessary using namespace

bc9dff8

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

klecki self-assigned this Jan 26, 2022

stiepan commented Jan 26, 2022

View reviewed changes

stiepan changed the title ~~Laplacian GPU operator~~ Add Laplacian GPU operator Jan 26, 2022

jantonguirao assigned szalpal Jan 27, 2022

NVIDIA deleted a comment from dali-automaton Jan 27, 2022

klecki reviewed Jan 28, 2022

View reviewed changes

stiepan added 2 commits January 31, 2022 13:18

Review remarks

85a2493

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Add slow attr to heavy Laplacian tests

51c6bb4

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan force-pushed the laplacian_gpu branch from bf9074d to 51c6bb4 Compare January 31, 2022 15:21

klecki approved these changes Feb 1, 2022

View reviewed changes

szalpal approved these changes Feb 7, 2022

View reviewed changes

stiepan merged commit 6c73ace into NVIDIA:main Feb 7, 2022

JanuszL mentioned this pull request Mar 30, 2022

DALI 2022 roadmap #3774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Laplacian GPU operator #3644

Add Laplacian GPU operator #3644

stiepan commented Jan 25, 2022 •

edited

Loading

stiepan commented Jan 25, 2022

dali-automaton commented Jan 25, 2022

dali-automaton commented Jan 25, 2022

stiepan Jan 26, 2022

klecki Jan 28, 2022

stiepan Jan 28, 2022

stiepan commented Jan 26, 2022

dali-automaton commented Jan 26, 2022

stiepan Jan 26, 2022

dali-automaton commented Jan 26, 2022

dali-automaton commented Jan 27, 2022

klecki left a comment

klecki Jan 28, 2022

stiepan Jan 28, 2022

klecki Jan 28, 2022

stiepan Jan 28, 2022

stiepan Jan 31, 2022

klecki Jan 28, 2022

stiepan Jan 31, 2022

klecki Jan 28, 2022

stiepan Jan 28, 2022

stiepan Jan 31, 2022

klecki Jan 28, 2022

stiepan Jan 31, 2022

klecki Jan 28, 2022

stiepan Jan 28, 2022

stiepan commented Jan 31, 2022

dali-automaton commented Jan 31, 2022

dali-automaton commented Jan 31, 2022

		@@ -0,0 +1,125 @@
		// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

		op_impl_uptr GetLaplacianGpuImpl(const OpSpec& spec,
		const DimDesc& dim_desc) {

Add Laplacian GPU operator #3644

Add Laplacian GPU operator #3644

Conversation

stiepan commented Jan 25, 2022 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

stiepan commented Jan 25, 2022

dali-automaton commented Jan 25, 2022

dali-automaton commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stiepan commented Jan 26, 2022

dali-automaton commented Jan 26, 2022

Choose a reason for hiding this comment

dali-automaton commented Jan 26, 2022

dali-automaton commented Jan 27, 2022

klecki 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

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

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

Choose a reason for hiding this comment

stiepan commented Jan 31, 2022

dali-automaton commented Jan 31, 2022

dali-automaton commented Jan 31, 2022

stiepan commented Jan 25, 2022 •

edited

Loading