Emboss - resolved review comments by RooseweltMcW · Pull Request #568 · r-abishek/rpp

RooseweltMcW · 2026-02-23T03:59:55Z

No description provided.

… into apr/emboss

r-abishek · 2026-02-24T04:14:28Z

@RooseweltMcW Shows merge conflicts

Copilot

Pull request overview

This PR removes OpenCL + legacy (RPPI/BatchPD) surface area, unifies/updates the public tensor APIs to support runtime backend selection (HOST vs HIP), and refreshes build + packaging + docs accordingly.

Changes:

Removed OpenCL backend + BatchPD OpenCL/HIP headers, CMake finder, and associated docs.
Unified multiple RPPT tensor APIs into single entrypoints that take RppBackend executionBackend.
Updated build/packaging metadata (version bump, Debian packaging templates, Sphinx deps, changelog, Doxygen inputs).

Reviewed changes

Copilot reviewed 50 out of 700 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
src/include/batch_pd/hip/hipoc_program.hpp	Removed BatchPD HIP program wrapper header.
src/include/batch_pd/hip/hipoc_kernel.hpp	Removed BatchPD HIP kernel wrapper header.
src/include/batch_pd/cl/temp_file.hpp	Removed BatchPD OpenCL temp-file helper.
src/include/batch_pd/cl/rpp_cl_common.hpp	Removed BatchPD OpenCL common utilities.
src/include/batch_pd/cl/oclkernel.hpp	Removed BatchPD OpenCL kernel wrapper.
src/include/batch_pd/cl/ocldeviceinfo.hpp	Removed BatchPD OpenCL device info helpers.
src/include/batch_pd/cl/load_file.hpp	Removed BatchPD OpenCL file loader helper.
src/include/batch_pd/cl/clhelper.hpp	Removed BatchPD OpenCL program/kernel helpers.
docs/sphinx/requirements.txt	Updated pinned doc toolchain versions.
docs/sphinx/requirements.in	Bumped `rocm-docs-core` input requirement.
docs/install/rpp-verify-install.rst	Removed OpenCL verification instructions.
docs/install/rpp-build-and-install.rst	Removed OpenCL build instructions.
cmake/FindOpenCL.cmake	Removed custom OpenCL finder module.
api/rppt_tensor_morphological_operations.h	Added HOST variants + unified erode/dilate entrypoints.
api/rppt_tensor_filter_augmentations.h	Unified filter APIs into backend-selectable entrypoints (plus doc updates).
api/rppt_tensor_data_exchange_operations.h	Unified data-exchange APIs into backend-selectable entrypoints.
api/rppt_tensor_bitwise_operations.h	Unified bitwise APIs into backend-selectable entrypoints.
api/rppt_tensor_audio_augmentations.h	Unified audio APIs into backend-selectable entrypoints.
api/rppt_tensor_arithmetic_operations.h	Unified arithmetic APIs into backend-selectable entrypoints.
api/rppt.h	Removed inclusion of legacy augmentation header.
api/rppi_statistical_operations.h	Removed deprecated RPPI statistical operations header.
api/rppi_morphological_transforms.h	Removed deprecated RPPI morphological transforms header.
api/rppi_logical_operations.h	Removed deprecated RPPI logical operations header.
api/rppi_legacy_augmentations.h	Removed legacy augmentation header.
api/rppi_filter_operations.h	Removed deprecated RPPI filter operations header.
api/rppi_color_model_conversions.h	Removed deprecated RPPI color conversions header.
api/rppi_arithmetic_operations.h	Removed deprecated RPPI arithmetic operations header.
api/rppi.h	Removed deprecated umbrella RPPI header.
api/rppdefs.h	Backend macro rename, removed OCL backend enum, added new status code + constants.
api/rpp_backend.h.in	Removed OpenCL backend CMake define.
api/rpp.h	Removed OpenCL queue typedefs + legacy include paths; clarified HIP/HOST-only handle docs.
README.md	Removed OpenCL backend build section.
DEBIAN/copyright.in	Switched to Debian copyright-format template content.
CMakeLists.txt	Removed OpenCL + legacy toggles; added TheRock handling + version bump + packaging hooks.
CHANGELOG.md	Documented API unification / backend selection + removal of legacy/OpenCL.
.Doxyfile	Removed RPPI headers from Doxygen `INPUT`.

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

Copilot · 2026-02-24T04:25:24Z

CMakeLists.txt

+if(USING_THE_ROCK)
+  if(NOT DEFINED ENV{HIP_DEVICE_LIB_PATH})
+    set(ENV{HIP_DEVICE_LIB_PATH} ${ROCM_PATH}/lib/llvm/amdgcn/bitcode)
+    set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE "HIP_DEVICE_LIB_PATH=${ROCM_PATH}/lib/llvm/amdgcn/bitcode")


RULE_LAUNCH_COMPILE expects a launcher command, not an environment-variable assignment. Setting it to HIP_DEVICE_LIB_PATH=... will typically be treated as an executable name and can break compilation. Use set(ENV{HIP_DEVICE_LIB_PATH} ...) alone or set RULE_LAUNCH_COMPILE to a real launcher (e.g., ${CMAKE_COMMAND} -E env HIP_DEVICE_LIB_PATH=... <compiler>), depending on the intended behavior.

Suggested change

set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE "HIP_DEVICE_LIB_PATH=${ROCM_PATH}/lib/llvm/amdgcn/bitcode")

Copilot · 2026-02-24T04:25:25Z

docs/sphinx/requirements.in

@@ -1 +1 @@
-rocm-docs-core[api_reference]==1.27.0
+rocm-docs-core[api_reference]==1.31.3


The extra name api_reference is inconsistent with the generated requirements.txt line that uses rocm-docs-core[api-reference]. Extras are normalized in some tooling, but not all; this mismatch can break reproducible lockfile generation (pip-compile) or dependency resolution. Align the extra name with the one actually provided by rocm-docs-core (and regenerate requirements.txt).

Suggested change

rocm-docs-core[api_reference]==1.31.3

rocm-docs-core[api-reference]==1.31.3

Copilot · 2026-02-24T04:25:25Z

api/rppt_tensor_morphological_operations.h

 #ifdef GPU_SUPPORT
 /*! \brief Erode augmentation on HIP backend for a NCHW/NHWC layout tensor


The unified rppt_erode(..., RppBackend executionBackend) declaration is guarded by #ifdef GPU_SUPPORT, which prevents HOST-only builds from using the unified API even though executionBackend can select RPP_HOST_BACKEND. This is inconsistent with other unified APIs in this PR (e.g., box/median/gaussian filter) that are not GPU-guarded. Consider declaring rppt_erode/rppt_dilate unconditionally and having the implementation return an appropriate status when HIP is requested but not compiled in.

Copilot · 2026-02-24T04:25:25Z

api/rppt_tensor_morphological_operations.h

+RppStatus rppt_erode(RppPtr_t srcPtr, RpptDescPtr srcDescPtr, RppPtr_t dstPtr, RpptDescPtr dstDescPtr, Rpp32u kernelSize, RpptROIPtr roiTensorPtrSrc, RpptRoiType roiType, rppHandle_t rppHandle, RppBackend executionBackend);
 #endif // GPU_SUPPORT


The unified rppt_erode(..., RppBackend executionBackend) declaration is guarded by #ifdef GPU_SUPPORT, which prevents HOST-only builds from using the unified API even though executionBackend can select RPP_HOST_BACKEND. This is inconsistent with other unified APIs in this PR (e.g., box/median/gaussian filter) that are not GPU-guarded. Consider declaring rppt_erode/rppt_dilate unconditionally and having the implementation return an appropriate status when HIP is requested but not compiled in.

Copilot · 2026-02-24T04:25:25Z

api/rppt_tensor_bitwise_operations.h

+ * \param [in] roiTensorPtrSrc ROI data in HIP memory (for HIP backend) or HOST memory (for HOST backend), 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))
 * \param [in] roiType ROI type used (RpptRoiType::XYWH or RpptRoiType::LTRB)
- * \param [in] rppHandle RPP HIP handle created with <tt>\ref rppCreateWithStreamAndBatchSize()</tt>
+ * \param [in] rppHandle RPP HIP/HOST handle created with <tt>\ref rppCreateWithBatchSize()</tt>


The documentation references rppCreateWithBatchSize(), but the public handle creation API shown elsewhere in this PR is rppCreate(...) (with a batch size parameter). If rppCreateWithBatchSize() is no longer part of the public API, this should be updated to prevent user confusion (similar references appear elsewhere, e.g., older rppCreateWithStreamAndBatchSize() wording).

Copilot · 2026-02-24T04:25:26Z

api/rppt_tensor_bitwise_operations.h

 */
-RppStatus rppt_bitwise_xor_gpu(RppPtr_t srcPtr1, RppPtr_t srcPtr2, RpptDescPtr srcDescPtr, RppPtr_t dstPtr, RpptDescPtr dstDescPtr, RpptROIPtr roiTensorPtrSrc, RpptRoiType roiType, rppHandle_t rppHandle);
-#endif // GPU_SUPPORT
+RppStatus rppt_bitwise_xor(RppPtr_t srcPtr1, RppPtr_t srcPtr2, RpptDescPtr srcDescPtr, RppPtr_t dstPtr, RpptDescPtr dstDescPtr, RpptROIPtr roiTensorPtrSrc, RpptRoiType roiType, rppHandle_t rppHandle, RppBackend executionBackend);


The documentation references rppCreateWithBatchSize(), but the public handle creation API shown elsewhere in this PR is rppCreate(...) (with a batch size parameter). If rppCreateWithBatchSize() is no longer part of the public API, this should be updated to prevent user confusion (similar references appear elsewhere, e.g., older rppCreateWithStreamAndBatchSize() wording).

Copilot · 2026-02-24T04:25:26Z

CHANGELOG.md

+* "Pinned buffer API support for HOST and HIP
+
+### Changed
+
+* AMDClag++ compiler has moved to `${ROCM_PATH}/lib/llvm/bin`


Fix the stray quote and missing closing quote/backtick in the 'Pinned buffer' bullet, and correct AMDClag++ to AMDClang++.

Suggested change

* "Pinned buffer API support for HOST and HIP

### Changed

* AMDClag++ compiler has moved to `${ROCM_PATH}/lib/llvm/bin`

* Pinned buffer API support for HOST and HIP

### Changed

* AMDClang++ compiler has moved to `${ROCM_PATH}/lib/llvm/bin`

Copilot · 2026-02-24T04:25:26Z

CHANGELOG.md

+* "Pinned buffer API support for HOST and HIP
+
+### Changed
+
+* AMDClag++ compiler has moved to `${ROCM_PATH}/lib/llvm/bin`


Fix the stray quote and missing closing quote/backtick in the 'Pinned buffer' bullet, and correct AMDClag++ to AMDClang++.

Suggested change

* "Pinned buffer API support for HOST and HIP

### Changed

* AMDClag++ compiler has moved to `${ROCM_PATH}/lib/llvm/bin`

* Pinned buffer API support for HOST and HIP

### Changed

* AMDClang++ compiler has moved to `${ROCM_PATH}/lib/llvm/bin`

Copilot · 2026-02-24T04:25:26Z

api/rppt_tensor_arithmetic_operations.h

 * \details This function performs the fmadd operation on a batch of 4D tensors.
 *          It multiplies each element of the source tensor by a corresponding element in the 'mulTensor',
 *          adds a corresponding element from the 'addTensor', and stores the result in the destination tensor.
 *          Support added for f32 -> f32 dataype.


Typo in documentation: dataype → datatype.

Copilot · 2026-02-24T04:25:26Z

DEBIAN/copyright.in

+Source: https://github.com/ROCm/@DEB_PACKAGE_NAME@
+Files: *
+License: @DEB_LICENSE@
+Copyright: @DEB_COPYRIGHT_YEAR@ Advanced Micro Devices, Inc. All rights Reserved.


This line is inconsistent with the MIT license text that follows (MIT is typically 'All rights reserved.' without the unusual capitalization, and Debian machine-readable copyright files usually separate Copyright: from License: blocks cleanly). Consider adjusting wording/capitalization and ensuring the machine-readable structure matches Debian's copyright-format expectations to avoid packaging lint issues.

…/emboss

… apr/emboss

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

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

Copilot · 2026-02-24T05:33:15Z

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

        sum_f8.f4[0] = FLOAT4_ZERO;
        sum_f8.f4[1] = FLOAT4_ZERO;
-        if ((id_x_i >= roiBeginX) && ((id_x_i + 7 + padLength) < roiWidth) && (id_y_i >= roiBeginY) && (id_y_i < roiHeight))
+        if ((id_x_i >= 0) && ((id_x_i + 7 + padLength) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))


Inconsistent boundary condition check. This line still uses padLength in the expression ((id_x_i + 7 + padLength) < roiWidth), while all other similar checks in this PR have been updated to use ((id_x_i + 7) < roiWidth). This inconsistency will cause incorrect behavior for the 7x7 emboss filter. The padLength should be removed from this condition.

Suggested change

if ((id_x_i >= 0) && ((id_x_i + 7 + padLength) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))

if ((id_x_i >= 0) && ((id_x_i + 7) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))

Modified, Done.

Copilot · 2026-02-24T05:33:16Z

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

        sum_f8.f4[0] = FLOAT4_ZERO;
        sum_f8.f4[1] = FLOAT4_ZERO;
-        if ((id_x_i >= roiBeginX) && ((id_x_i + 7 + padLength) < roiWidth) && (id_y_i >= roiBeginY) && (id_y_i < roiHeight))
+        if ((id_x_i >= 0) && ((id_x_i + 7) < 0) && (id_y_i >= roiBeginY) && (id_y_i < roiHeight))


Critical bug in the boundary condition check. The condition ((id_x_i + 7) < 0) is incorrect and will always be false for valid coordinates. This should be ((id_x_i + 7) < roiWidth) to match the pattern used in other parts of the code. Additionally, id_y_i >= roiBeginY should be id_y_i >= 0 to be consistent with the coordinate system changes made throughout this PR.

Suggested change

if ((id_x_i >= 0) && ((id_x_i + 7) < 0) && (id_y_i >= roiBeginY) && (id_y_i < roiHeight))

if ((id_x_i >= 0) && ((id_x_i + 7) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))

Modified, Done.

HazarathKumarM added 4 commits December 9, 2025 04:44

Merge branch 'apr/emboss_dropout' of https://github.com/Srihari-mcw/rpp…

0da14f2

… into apr/emboss

Updated ROI for emboss

44602c2

merge ar/emboss to apr/emboss

14648b9

Resolved copilot review comments and removed bias parameter

f9202ca

r-abishek changed the base branch from ar/emboss to master February 24, 2026 04:14

r-abishek changed the base branch from master to ar/emboss February 24, 2026 04:14

r-abishek requested a review from Copilot February 24, 2026 04:23

r-abishek assigned RooseweltMcW Feb 24, 2026

r-abishek added the minor fix Minor implementation fixes label Feb 24, 2026

Copilot AI reviewed Feb 24, 2026

View reviewed changes

HazarathKumarM added 2 commits February 23, 2026 23:35

Merge branch 'ar/emboss' of https://github.com/r-abishek/rpp into apr…

0a66edf

…/emboss

Merge branch 'apr/emboss' of https://github.com/RooseweltMcW/rpp into…

3a26880

… apr/emboss

r-abishek requested a review from Copilot February 24, 2026 05:29

Copilot started reviewing on behalf of r-abishek February 24, 2026 05:30 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

Resolved copilot review comments

3889db7

r-abishek approved these changes Feb 25, 2026

View reviewed changes

r-abishek merged commit c4afb65 into r-abishek:ar/emboss Feb 25, 2026

		@@ -1 +1 @@
		rocm-docs-core[api_reference]==1.27.0
		rocm-docs-core[api_reference]==1.31.3

	rocm-docs-core[api_reference]==1.31.3
	rocm-docs-core[api-reference]==1.31.3

		#ifdef GPU_SUPPORT
		/*! \brief Erode augmentation on HIP backend for a NCHW/NHWC layout tensor

		RppStatus rppt_erode(RppPtr_t srcPtr, RpptDescPtr srcDescPtr, RppPtr_t dstPtr, RpptDescPtr dstDescPtr, Rpp32u kernelSize, RpptROIPtr roiTensorPtrSrc, RpptRoiType roiType, rppHandle_t rppHandle, RppBackend executionBackend);
		#endif // GPU_SUPPORT

	if ((id_x_i >= 0) && ((id_x_i + 7 + padLength) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))
	if ((id_x_i >= 0) && ((id_x_i + 7) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))

	if ((id_x_i >= 0) && ((id_x_i + 7) < 0) && (id_y_i >= roiBeginY) && (id_y_i < roiHeight))
	if ((id_x_i >= 0) && ((id_x_i + 7) < roiWidth) && (id_y_i >= 0) && (id_y_i < roiHeight))

Conversation

RooseweltMcW commented Feb 23, 2026

Uh oh!

r-abishek commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

RooseweltMcW Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

RooseweltMcW Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants