Channel Dropout Resolved review comments by RooseweltMcW · Pull Request #541 · r-abishek/rpp

RooseweltMcW · 2025-12-03T07:45:40Z

No description provided.

Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

* Solarize HIP and HOST implementation * cleanup the code and fix pkd3-pkd3 performance * Add golden output and doxygen comments * Add cheks for Threshold param * modified case num for solarize * minor fix * fix load/store calls * Address review comments * minor fix --------- Co-authored-by: HazarathKumarM <hazarathkumar@multicorewareinc.com> Co-authored-by: Maddisetty <hmaddise@ctr2-alola-login-01.amd.com> Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

…ocs/sphinx (ROCm#622) Bumps [rocm-docs-core[api_reference]](https://github.com/ROCm/rocm-docs-core) from 1.24.1 to 1.25.0. - [Release notes](https://github.com/ROCm/rocm-docs-core/releases) - [Changelog](https://github.com/ROCm/rocm-docs-core/blob/v1.25.0/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v1.24.1...v1.25.0) --- updated-dependencies: - dependency-name: rocm-docs-core[api_reference] dependency-version: 1.25.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Initial hip memory fixes * Further hip warning fixes * Default build warning fixes - group III --------- Co-authored-by: Srihari-mcw <srihari@multicorewareinc.com> Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

…ocs/sphinx (ROCm#623) Bumps [rocm-docs-core[api_reference]](https://github.com/ROCm/rocm-docs-core) from 1.25.0 to 1.26.0. - [Release notes](https://github.com/ROCm/rocm-docs-core/releases) - [Changelog](https://github.com/ROCm/rocm-docs-core/blob/v1.26.0/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v1.25.0...v1.26.0) --- updated-dependencies: - dependency-name: rocm-docs-core[api_reference] dependency-version: 1.26.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Initial changes for tensor sum for PLN1 and PKD3 variants U8 * Make changes for rest of the bit depths and layouts * Remove unused pack functions * Adjust comments * Initial cleanup * Further cleanup * Remove unused functions * Address copilot comments * Further standardize comments * Standardize comments spacing in functions affected --------- Co-authored-by: Srihari-mcw <srihari@multicorewareinc.com> Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

* removed numbers for bitdepths and output toggle * Add layout enum and replicate changes to HOST testsuite * Fix copilot review comments * resolve review comments * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * minor bugfix * minor bugfix * minor fix --------- Co-authored-by: HazarathKumarM <hazarathkumar@multicorewareinc.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

…ocs/sphinx (ROCm#632) Bumps [rocm-docs-core[api_reference]](https://github.com/ROCm/rocm-docs-core) from 1.26.0 to 1.27.0. - [Release notes](https://github.com/ROCm/rocm-docs-core/releases) - [Changelog](https://github.com/ROCm/rocm-docs-core/blob/develop/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v1.26.0...v1.27.0) --- updated-dependencies: - dependency-name: rocm-docs-core[api_reference] dependency-version: 1.27.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ocs/sphinx (ROCm#622) Bumps [rocm-docs-core[api_reference]](https://github.com/ROCm/rocm-docs-core) from 1.24.1 to 1.25.0. - [Release notes](https://github.com/ROCm/rocm-docs-core/releases) - [Changelog](https://github.com/ROCm/rocm-docs-core/blob/v1.25.0/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v1.24.1...v1.25.0) --- updated-dependencies: - dependency-name: rocm-docs-core[api_reference] dependency-version: 1.25.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…annel_dropout

Copilot

Pull request overview

This PR refactors the channel dropout implementation to resolve review comments by moving mask generation logic from kernel/executor functions to the top-level API functions. The key change is replacing the randomSeed boolean parameter with a pre-generated maskPtr, making the implementation more efficient and separating concerns between mask generation and application.

Key Changes:

Moved channel dropout mask generation from kernel/executor functions to the top-level rppt_channel_dropout_host and rppt_channel_dropout_gpu functions
Changed function signatures to pass pre-generated masks instead of a random seed boolean
Added error code capture in test files for better error handling

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
utilities/test_suite/HOST/Tensor_image_host.cpp	Added error code capture for channel dropout host function call
utilities/test_suite/HIP/Tensor_image_hip.cpp	Added error code capture for channel dropout GPU function call
src/modules/tensor/rppt_tensor_effects_augmentations.cpp	Moved mask generation logic to top-level functions; updated function calls to pass mask pointers instead of random seed
src/modules/tensor/hip/kernel/channel_dropout.cpp	Removed mask generation code from kernel executor; updated to receive pre-generated masks
src/modules/tensor/cpu/kernel/channel_dropout.cpp	Removed mask generation from kernel function; renamed local variable for clarity; updated to use pre-generated masks
src/include/tensor/host_tensor_executors.hpp	Updated function signature to accept mask pointer instead of random seed boolean
src/include/tensor/hip_tensor_executors.hpp	Updated function signature to accept device mask pointer instead of random seed boolean

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

HazarathKumarM · 2025-12-11T11:55:21Z

api/rppt_tensor_effects_augmentations.h

 * \param [in] dstDescPtr destination tensor descriptor (Restrictions - numDims = 4, offsetInBytes >= 0, dataType = U8/F16/F32/I8, layout = NCHW/NHWC, c = same as that of srcDescPtr)
- * \param [in] dropoutProbability dropout probability for channel dropout calculation (1D Rpp32f tensor in HOST memory, of size batchSize with 0 <= dropProb[i] <= 1 for each image in batch)
- * \param [in] randomSeed randomSeed single bool to control the random number generator's seed ( 0 - Fixed seed for QA , 1 - random seed generated for randomness)
+ * \param [in] channelTensor channel dropout tensor (1D Rpp32f tensor in HOST memory, of size batchSize * channels Values must be 0.0f (Drop) or 1.0f (Keep) for each channel of each image in batch)


change this variable to dropoutTensor, applicable to all instances

HazarathKumarM · 2025-12-11T12:51:54Z

api/rppt_tensor_effects_augmentations.h

 * \param [in] dstDescPtr destination tensor descriptor (Restrictions - numDims = 4, offsetInBytes >= 0, dataType = U8/F16/F32/I8, layout = NCHW/NHWC, c = same as that of srcDescPtr)
- * \param [in] dropoutProbability dropout probability for channel dropout calculation (1D Rpp32f tensor in HOST memory, of size batchSize with 0 <= dropProb[i] <= 1 for each image in batch)
- * \param [in] randomSeed randomSeed single bool to control the random number generator's seed ( 0 - Fixed seed for QA , 1 - random seed generated for randomness)
+ * \param [in] dropoutTensor channel dropout tensor (1D Rpp32f tensor in HOST memory, of size batchSize * channels Values must be 0.0f (Drop) or 1.0f (Keep) for each channel of each image in batch)


check this comment , it shouldn't be HOST memory

Also should be Rpp8u instead of Rpp32f

Srihari-mcw · 2025-12-11T17:06:05Z

api/rppt_tensor_effects_augmentations.h

 * \param [in] dstDescPtr destination tensor descriptor (Restrictions - numDims = 4, offsetInBytes >= 0, dataType = U8/F16/F32/I8, layout = NCHW/NHWC, c = same as that of srcDescPtr)
- * \param [in] dropoutProbability dropout probability for channel dropout calculation (1D Rpp32f tensor in HOST memory, of size batchSize with 0 <= dropProb[i] <= 1 for each image in batch)
- * \param [in] randomSeed randomSeed single bool to control the random number generator's seed ( 0 - Fixed seed for QA , 1 - random seed generated for randomness)
+ * \param [in] dropoutTensor channel dropout tensor (1D Rpp32f tensor in HOST memory, of size batchSize * channels Values must be 0.0f (Drop) or 1.0f (Keep) for each channel of each image in batch)


Srihari-mcw · 2025-12-11T17:09:07Z

src/include/tensor/hip_tensor_executors.hpp

                                          RpptDescPtr dstDescPtr,
-                                          Rpp32f *dropoutProbability,
-                                          bool randomSeed,
+                                          Rpp8u *d_dropoutTensor,


Is the convention to name d_dropoutTensor - Doubt it, pls check

Used similar naming convention used in fog kernel.

d_ will be used only if the memory allocated to the tensor is completely device only memory. Please verify modify this if needed

Srihari-mcw · 2025-12-11T17:16:42Z

utilities/test_suite/HIP/Tensor_image_hip.cpp

+    Rpp8u *dropoutTensor = nullptr;
    if(testCase == CHANNEL_DROPOUT)
-        CHECK_RETURN_STATUS(hipHostMalloc(&dropoutProbability, batchSize * sizeof(Rpp32f)));
+        CHECK_RETURN_STATUS(hipHostMalloc(&dropoutTensor, batchSize * 3 * sizeof(Rpp32f)));


Maybe this should be batchSize * srcDescPtr->c, I have a doubt, is the channel dropout color image specific? Is there any condition for this somewhere?

Yes, but for PLN1 it is also included such a way that it runs and keeps one channel and if the user wants to based on the probability it can now also be black image.

in that case one value is enough, right? why creating a tensor with 3 values

this need to be checked

Srihari-mcw

Pls make the requested changes/ reply why the current code is valid

HazarathKumarM · 2025-12-12T08:56:40Z

api/rppt_tensor_effects_augmentations.h

 * \param [in] dstDescPtr destination tensor descriptor (Restrictions - numDims = 4, offsetInBytes >= 0, dataType = U8/F16/F32/I8, layout = NCHW/NHWC, c = same as that of srcDescPtr)
- * \param [in] dropoutProbability dropout probability for channel dropout calculation (1D Rpp32f tensor in HOST memory, of size batchSize with 0 <= dropProb[i] <= 1 for each image in batch)
- * \param [in] randomSeed randomSeed single bool to control the random number generator's seed ( 0 - Fixed seed for QA , 1 - random seed generated for randomness)
+ * \param [in] dropoutTensor channel dropout tensor (1D Rpp8u tensor in HOST memory, of size batchSize * channels Values must be 0.0f (Drop) or 1.0f (Keep) for each channel of each image in batch)


in the comment it is said it is a rpp8u tensor, but you are mentioning float values like 0.0f and 1.0f. Please change them to 0 or 1

HazarathKumarM · 2025-12-12T08:58:10Z

src/include/tensor/hip_tensor_executors.hpp

                                          RpptDescPtr dstDescPtr,
-                                          Rpp32f *dropoutProbability,
-                                          bool randomSeed,
+                                          Rpp8u *d_dropoutTensor,


d_ will be used only if the memory allocated to the tensor is completely device only memory. Please verify modify this if needed

HazarathKumarM · 2025-12-12T09:00:04Z

src/modules/tensor/rppt_tensor_effects_augmentations.cpp

-
    if (srcDescPtr->dataType != dstDescPtr->dataType) return RPP_ERROR_INVALID_SRC_OR_DST_DATATYPE;

+    Rpp8u *d_dropoutTensor = reinterpret_cast<Rpp8u *>(rpp::deref(rppHandle).GetInitHandle()->mem.mgpu.scratchBufferHip.floatmem);


any reason why we are not passing pinned memory / HIP memory itself from the test suite?

this will not work if we need an unified api for this kernel

Modified, Done.

HazarathKumarM · 2025-12-12T09:00:19Z

api/rppt_tensor_effects_augmentations.h

 * \param [in] dstDescPtr destination tensor descriptor (Restrictions - numDims = 4, offsetInBytes >= 0, dataType = U8/F16/F32/I8, layout = NCHW/NHWC, c = same as that of srcDescPtr)
- * \param [in] dropoutProbability dropout probability for channel dropout calculation (1D Rpp32f tensor in HOST memory, of size batchSize with 0 <= dropProb[i] <= 1 for each image in batch)
- * \param [in] randomSeed randomSeed single bool to control the random number generator's seed ( 0 - Fixed seed for QA , 1 - random seed generated for randomness)
+ * \param [in] dropoutTensor channel dropout tensor (1D Rpp8u tensor in HOST memory, of size batchSize * channels Values must be 0.0f (Drop) or 1.0f (Keep) for each channel of each image in batch)


same comment as HOST

still the description say it is a host buffer. please modify it

HazarathKumarM · 2025-12-12T09:01:56Z

utilities/test_suite/HOST/Tensor_image_host.cpp

+                    Rpp32f seed = qaFlag ? DROPOUT_FIXED_SEED : std::random_device{}();
                    for (i = 0; i < batchSize; i++)
                        dropoutProbability[i] = 0.4f;
+                    Rpp8u dropoutTensor[batchSize * 3];


here are also remove hardcoded 3 use channels instead

HazarathKumarM · 2025-12-15T12:35:53Z

api/rppt_tensor_effects_augmentations.h

- * \param [in] dropoutProbability dropout probability for channel dropout calculation (1D Rpp32f tensor in HOST memory, of size batchSize with 0 <= dropProb[i] <= 1 for each image in batch)
- * \param [in] randomSeed randomSeed single bool to control the random number generator's seed ( 0 - Fixed seed for QA , 1 - random seed generated for randomness)
+ * \param [in] dropoutTensor channel dropout tensor (1D Rpp8u tensor in HIP memory, of size batchSize * channels Values must be 0 (Drop) or 1 (Keep) for each channel of each image in batch)
 * \param [in] roiTensorPtrSrc ROI data in HIP memory, for each image in source tensor (2D tensor of size batchSize * 4, in either format - XYWH(xy.x, xy.y, roiWidth, roiHeight) or LTRB(lt.x, lt.y, rb.x, rb.y))


is it pinned or HIP?

Modified, Done

HazarathKumarM · 2025-12-15T12:37:51Z

utilities/test_suite/HIP/Tensor_image_hip.cpp

+    Rpp8u *dropoutTensor = nullptr;
    if(testCase == CHANNEL_DROPOUT)
-        CHECK_RETURN_STATUS(hipHostMalloc(&dropoutProbability, batchSize * sizeof(Rpp32f)));
+        CHECK_RETURN_STATUS(hipHostMalloc(&dropoutTensor, batchSize * 3 * sizeof(Rpp32f)));


this need to be checked

…_dropout

RooseweltMcW · 2025-12-16T05:39:40Z

@r-abishek resolved all merge conflicts and review comments.

Srihari-mcw · 2025-12-16T05:41:52Z

src/modules/tensor/hip/kernel/channel_dropout.cpp

                                               T *dstPtr,
                                               uint2 dstStridesNH,
-                                               uint8_t *channelMask,
+                                               uint8_t *dropoutTensor,


uint8_t is supposed to be the datatype. I understand its equivalent to Rpp8u but is it following convention from other kernel files?

Took reference from posterize kernel and modified the datatype to Rpp8u, Done.

r-abishek · 2025-12-17T02:01:53Z

src/include/tensor/hip_tensor_executors.hpp

                                          RpptDescPtr dstDescPtr,
-                                          Rpp32f *dropoutProbability,
-                                          bool randomSeed,
+                                          Rpp8u *dropoutTensor,


@Srihari-mcw or @HazarathKumarM Can't it be a bool array?

r-abishek

Pls only check the comment on using bool

Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

arvindcheru and others added 20 commits October 29, 2025 03:06

Packaging - Remove Meta Package dependency (ROCm#610)

2183188

Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

Test Package - Add cmake deps (ROCm#617)

316bb3f

Warning fixes for HIP Backend - Check return status (ROCm#606)

38614fd

* Initial hip memory fixes * Further hip warning fixes * Default build warning fixes - group III --------- Co-authored-by: Srihari-mcw <srihari@multicorewareinc.com> Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

ROCm 7.1 - Changelog updates (ROCm#625)

5243e23

CXX Compiler - Support TheRock (ROCm#630)

750f0ea

Merge remote-tracking branch 'tot/develop' into apr/channel_dropout

67c0b95

Modified enum changed for the test suite for dropout

41baa4a

ROCm 7.1 - Changelog updates (ROCm#625)

2e8ac16

Merge remote-tracking branch 'abishek/ar/dropout_channel' into apr/ch…

37d197f

…annel_dropout

Merge remote-tracking branch 'tot/develop' into apr/channel_dropout

861ec7a

updated channel dropout and resolved review comments

6d191fc

Merge remote-tracking branch 'abishek/ar/dropout_channel' into apr/ch…

527e957

…annel_dropout

Updated random mask in API level

1bacbad

r-abishek requested a review from Copilot December 3, 2025 17:47

r-abishek assigned HazarathKumarM and RooseweltMcW Dec 3, 2025

Copilot AI reviewed Dec 3, 2025

View reviewed changes

r-abishek added the minor fix Minor implementation fixes label Dec 3, 2025

Srihari-mcw pushed a commit to Srihari-mcw/rpp that referenced this pull request Dec 8, 2025

Copyright - for missing files (r-abishek#541)

3f5ee6c

Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>

HazarathKumarM added 2 commits December 10, 2025 05:26

Updated seed to be passed from test suite

2c8e572

Updated Random generator in test suite level

0573be4

HazarathKumarM reviewed Dec 11, 2025

View reviewed changes

updated name for every instances

120af19

HazarathKumarM reviewed Dec 11, 2025

View reviewed changes

Srihari-mcw reviewed Dec 11, 2025

View reviewed changes

Srihari-mcw requested changes Dec 11, 2025

View reviewed changes

Resolved review comments

45d301c

HazarathKumarM reviewed Dec 12, 2025

View reviewed changes

HazarathKumarM added 2 commits December 12, 2025 05:05

Merge remote-tracking branch 'origin/develop' into apr/channel_dropout

cdf05e0

Resolved Review comments and modified HIP code

7b050eb

HazarathKumarM changed the base branch from ar/dropout_channel to master December 12, 2025 11:23

HazarathKumarM changed the base branch from master to ar/dropout_channel December 12, 2025 11:24

HazarathKumarM reviewed Dec 15, 2025

View reviewed changes

Updated and resolved review comments

c7454a5

r-abishek changed the base branch from ar/dropout_channel to develop December 16, 2025 04:11

HazarathKumarM changed the base branch from develop to master December 16, 2025 04:12

r-abishek changed the base branch from master to ar/dropout_channel December 16, 2025 04:12

Merge remote-tracking branch 'ar/ar/dropout_channel' into apr/channel…

faeaf5c

…_dropout

Srihari-mcw reviewed Dec 16, 2025

View reviewed changes

Modified datatype for channel dropout Hip backend

6e319e9

r-abishek reviewed Dec 17, 2025

View reviewed changes

r-abishek approved these changes Dec 17, 2025

View reviewed changes

r-abishek merged commit 8a1f90e into r-abishek:ar/dropout_channel Dec 17, 2025

ManasaDattaT pushed a commit to ManasaDattaT/rpp that referenced this pull request Dec 19, 2025

Copyright - for missing files (r-abishek#541)

2f94257

Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com>


		if (srcDescPtr->dataType != dstDescPtr->dataType) return RPP_ERROR_INVALID_SRC_OR_DST_DATATYPE;

		Rpp8u d_dropoutTensor = reinterpret_cast<Rpp8u >(rpp::deref(rppHandle).GetInitHandle()->mem.mgpu.scratchBufferHip.floatmem);

Conversation

RooseweltMcW commented Dec 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Srihari-mcw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RooseweltMcW commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r-abishek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees