RPP - Enhanced Handle Initialization and API Optimization by Dineshbabu-Ravichandran · Pull Request #386 · r-abishek/rpp

Dineshbabu-Ravichandran · 2025-01-15T05:52:54Z

Added rppCreate and rppDestroy for handle initialization and destruction.
Removed unused handle APIs and constructor functions.

…ndle_modification

r-abishek

@Dineshbabu-Ravichandran Please address comments.
All changes after addressing comments are okay except these 4 files for this PR:

handle.hpp
handleocl.cpp
handlehost.cpp
handlehip.cpp

r-abishek · 2025-01-16T02:18:23Z

include/rppdefs.h

    RPP_ERROR_INVALID_DST_DIMS          = -24
 } RppStatus;

+typedef enum


Please add docs something like:

/*! \brief RPP RppBackend type enums * \ingroup group_rppdefs */

r-abishek · 2025-01-16T02:21:47Z

include/rpp.h

+/*! \brief Creates RPP handle for HOST and HIP batch processing.
 * \details Function to create a RPP handle for a batch. To be called in the beginning to initialize the RPP environment.
 * \param [in] handle A pointer to RPP handle of type <tt> \ref rppHandle_t</tt>.
 * \param [in] nBatchSize Batch size.


The \param [in] needs to be added for all new arguments

r-abishek · 2025-01-16T02:25:40Z

include/rpp.h

 * \retval rppStatusUnsupportedOp
 */
-extern "C" SHARED_PUBLIC rppStatus_t rppCreateWithBatchSize(rppHandle_t* handle, size_t nBatchSize, Rpp32u numThreads = 0);
+extern "C" SHARED_PUBLIC rppStatus_t rppCreate(rppHandle_t* handle, size_t nBatchSize, void* numThreadsOrStream, RppBackend backend = RppBackend::RPP_HOST_BACKEND);


Are we sure none of the voxel hip kernels use openmp and hip streams? Is there any situation where we need both threads and streams? Pls verify

Yes I verified , There is no constructor of handle that accepts stream and numThread at the same time and also there is no api named rppCreateWithStreamAndThread .

r-abishek · 2025-01-16T02:27:13Z

include/rpp.h

-extern "C" SHARED_PUBLIC rppStatus_t rppCreate(rppHandle_t* handle);
-
-/*! \brief Creates RPP handle for HOST batch processing.
+/*! \brief Creates RPP handle for HOST and HIP batch processing.


"Creates RPP handle for HOST/HIP/OCL backend batch processing."

r-abishek · 2025-01-16T02:27:29Z

include/rpp.h


-/*! \brief Destory RPP handle.
- * \details Function to destroy a RPP handle. To be called in the end to break down the RPP environment.
+/*! \brief Destory RPP HOST/GPU handle.


"Destroys RPP handle for HOST/HIP/OCL backend batch processing."

r-abishek · 2025-01-16T02:57:15Z

src/modules/handle_api.cpp

-    return rpp::try_([&] { rpp_destroy_object(handle); });
+    if(backend == RppBackend::RPP_HIP_BACKEND || backend == RppBackend::RPP_OCL_BACKEND)
+    {
+#if GPU_SUPPORT  


The #if GPU_SUPPORT could be outside with an else-if inside.
Also, why do we need a static_cast of a reinterpret_cast?

if(backend == RppBackend::RPP_HOST_BACKEND) { Rpp32u numThreads = *reinterpret_cast<Rpp32u*>(numThreadsOrStream); return rpp::try_([&] { rpp::deref(handle) = new rpp::Handle(nBatchSize, numThreads); }); } #if GPU_SUPPORT else if ((backend == RppBackend::RPP_HIP_BACKEND) || (backend == RppBackend::RPP_OCL_BACKEND)) { rppAcceleratorQueue_t deviceQueue = reinterpret_cast<rppAcceleratorQueue_t>(numThreadsOrStream); return rpp::try_([&] { rpp::deref(handle) = new rpp::Handle(deviceQueue, nBatchSize); }); } #endif // GPU_SUPPORT else { return rppStatusNotImplemented; }

The above is okay except please change the order of arguments in "new rpp::Handle" so that nBatchSize is always first. There is a discrepancy for host and device above.

If I directly reinterpret_cast to Rpp32u . I am getting below error :

error: cast from pointer to smaller type 'Rpp32u' (aka 'unsigned int') loses information 33 | Rpp32u numThreads = (reinterpret_cast<Rpp32u>(numThreadsOrStream)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated when compiling for gfx908.

I changed the argument order for HIP and OCL .

r-abishek · 2025-01-16T03:42:40Z

src/modules/hip/handlehip.cpp

    return device;
 }

-void set_device(int id)


Reasons to remove these 5?

set_device()

set_default_device()

create_stream()

Handle::Handle(rppAcceleratorQueue_t stream) : impl(new HandleImpl())

Handle::Handle() : impl(new HandleImpl())

void Handle::SetStream(rppAcceleratorQueue_t streamID) const

set_device() function is used inside the set_default_device()

set_default_device() and create_stream() is used in the default constructor of handle with no argument Handle::Handle() : impl(new HandleImpl())

We are using the constructor of handle with batchsize and thread as a arguments and batchsize and stream as a arguments . SO I removed the remaining constructors (default constructor and constructor with only stream ).

setStream() - not called. I searched through global search and removed it .

r-abishek · 2025-01-16T03:43:17Z

src/modules/handlehost.cpp

    impl->PreInitializeBufferCPU();
 }

-Handle::Handle() : impl(new HandleImpl())


Same on handlehost. Reasons to remove?

Handle::Handle() : impl(new HandleImpl())

Same for the HOST handle constructor .

I keep the handle constructor with batchsize and threadnum as a argument.

So I removed the remaining constructor (default constructor).

r-abishek · 2025-01-16T03:45:21Z

src/modules/cl/handleocl.cpp

    impl->PreInitializeBuffer();
 }

-Handle::Handle(rppAcceleratorQueue_t stream) : impl(new HandleImpl())


Same for the handleocl file. Reasons to remove?

Handle::Handle(rppAcceleratorQueue_t stream) : impl(new HandleImpl())

Handle::Handle() : impl(new HandleImpl())

Handle::SetStream(rppAcceleratorQueue_t streamID) const

Same for the OCL Constructor also

Keep only constructor with batchSize and stream as a argument to constructor .

Removed the remaining constructor (default constructor and constructor with stream as a argument )

SetStream() is not called on any file . I searched via global search and removed it .

r-abishek · 2025-01-16T03:46:01Z

src/include/common/rpp/handle.hpp

 {
-    Handle();
    Handle(size_t nBatchSize, Rpp32u numThreads = 0);
-    Handle(Handle&&) noexcept;


Same comment for changes in this file

I removed the declaration of the removed functions on handlehost , handlehip and handleocl file on handle file .

r-abishek

lgtm

…oups (r-abishek#386)

HazarathKumarM and others added 6 commits January 9, 2025 08:14

Changed to rppcreateHost and rppCreateGPU

7923722

Added rppCreate for HOST and HIP

acdb95e

Completely removed RppCreateHOST and rppCreateHIP on BatchPD.

25f4d4f

Destroy HOST and GPU is removed

bfd1941

Merge branch 'develop' into db/handle_modification

e9f107f

Merge branch 'develop' of https://github.com/r-abishek/rpp into db/ha…

856e992

…ndle_modification

r-abishek requested changes Jan 16, 2025

View reviewed changes

r-abishek assigned Dineshbabu-Ravichandran Jan 16, 2025

r-abishek added the enhancement New feature or request label Jan 16, 2025

Changes based on review comments

ef29062

r-abishek approved these changes Jan 22, 2025

View reviewed changes

r-abishek added this to the sow12ms1 milestone Jan 22, 2025

r-abishek changed the base branch from develop to ar/rpp_restructure_handle January 22, 2025 08:24

r-abishek merged commit fd7be3f into r-abishek:ar/rpp_restructure_handle Jan 22, 2025

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

Docs - doxygen fix for docs not showing arithmetic, logical, audio gr…

3ee01d7

…oups (r-abishek#386)

Conversation

Dineshbabu-Ravichandran commented Jan 15, 2025

Uh oh!

r-abishek 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dineshbabu-Ravichandran Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dineshbabu-Ravichandran Jan 16, 2025 •

edited

Loading