Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audio HIP PR5 - Preemphasis Filter HIP Support #270

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1147bfe
Update CMakeLists.txt
kiritigowda Apr 12, 2024
5e3fc7a
Bump rocm-docs-core[api_reference] from 0.38.1 to 1.0.0 in /docs/sphi…
dependabot[bot] Apr 18, 2024
fe1a3e6
Add Pre emphasis filter HIP implementation
sampath1117 Apr 25, 2024
77e14ef
Minor common-fixes for HIP (#345)
r-abishek May 7, 2024
34f3f6d
Readme Updates: --usecase=rocm (#349)
kiritigowda May 8, 2024
ab52683
RPP Tensor Audio Support - Spectrogram (#312)
r-abishek May 8, 2024
ee0d6fe
Update CHANGELOG.md (#352)
r-abishek May 8, 2024
2decd32
RPP Tensor Audio Support - Slice (#325)
r-abishek May 8, 2024
30ce1d6
RPP Tensor Audio Support - MelFilterBank (#332)
r-abishek May 8, 2024
64ae74f
RPP Tensor Normalize ND on HOST and HIP (#335)
r-abishek May 9, 2024
1a3015c
SWDEV-459739 - Remove the package obsolete setting (#353)
raramakr May 9, 2024
e5865f9
clean up the code
sampath1117 May 9, 2024
c87f98b
Merge remote-tracking branch 'abishek/develop' into sr/pre_emphasis_f…
sampath1117 May 9, 2024
64ca5a3
Address review comments
sampath1117 May 17, 2024
5eeb4b1
Merge remote-tracking branch 'abishek/develop' into sr/pre_emphasis_f…
HazarathKumarM May 30, 2024
708160c
Resolve review comments and modify load and store in HIP kernel
HazarathKumarM Jun 27, 2024
7e4f3f1
Merge remote-tracking branch 'abishek/develop' into sr/pre_emphasis_f…
HazarathKumarM Jun 27, 2024
7e7af14
modified verify_output to have different cutoff for HIP and HOST back…
HazarathKumarM Jun 27, 2024
290449e
cleanup
HazarathKumarM Jul 10, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .readthedocs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@ python:
build:
os: ubuntu-22.04
tools:
python: "3.8"
python: "3.10"
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ set(CMAKE_CXX_COMPILER clang++)
set(CMAKE_CXX_STANDARD 17)

# RPP Version
set(VERSION "1.6.0")
set(VERSION "1.7.0")

# Set Project Version and Language
project(rpp VERSION ${VERSION} LANGUAGES CXX)
Expand Down
2 changes: 1 addition & 1 deletion docs/sphinx/requirements.in
Original file line number Diff line number Diff line change
@@ -1 +1 @@
rocm-docs-core[api_reference]==0.38.1
rocm-docs-core[api_reference]==1.0.0
2 changes: 1 addition & 1 deletion docs/sphinx/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ requests==2.28.2
# via
# pygithub
# sphinx
rocm-docs-core[api-reference]==0.38.1
rocm-docs-core[api-reference]==1.0.0
# via -r requirements.in
smmap==5.0.0
# via gitdb
Expand Down
3 changes: 3 additions & 0 deletions include/rppt_tensor_audio_augmentations.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ RppStatus rppt_to_decibels_host(RppPtr_t srcPtr, RpptDescPtr srcDescPtr, RppPtr_
* \retval RPP_ERROR* Unsuccessful completion.
*/
RppStatus rppt_pre_emphasis_filter_host(RppPtr_t srcPtr, RpptDescPtr srcDescPtr, RppPtr_t dstPtr, RpptDescPtr dstDescPtr, Rpp32s *srcLengthTensor, Rpp32f *coeffTensor, RpptAudioBorderType borderType, rppHandle_t rppHandle);
#ifdef GPU_SUPPORT
RppStatus rppt_pre_emphasis_filter_gpu(RppPtr_t srcPtr, RpptDescPtr srcDescPtr, RppPtr_t dstPtr, RpptDescPtr dstDescPtr, Rpp32s *srcLengthTensor, Rpp32f *coeffTensor, RpptAudioBorderType borderType, rppHandle_t rppHandle);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the description for HIP API too
I have added similar description for ToDecibels HIP API
https://github.com/sampath1117/rpp/blob/sr/to_decibels_hip/include/rppt_tensor_audio_augmentations.h#L83

In recent RPP changes, we are following this approach of adding description for both api's

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#endif // GPU_SUPPORT

/*! \brief Down Mixing augmentation on HOST backend
* \details Down Mixing augmentation for audio data
Expand Down
6 changes: 6 additions & 0 deletions src/modules/hip/hip_tensor_audio_augmentations.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#ifndef HIP_TENSOR_AUDIO_HPP

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MACRO should match file name for this files
https://github.com/sampath1117/rpp/blob/sr/to_decibels_hip/src/modules/hip/hip_tensor_audio_augmentations.hpp

So it should be changed to HIP_TENSOR_AUDIO_AUGMENTATIONS_HPP

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#define HIP_TENSOR_AUDIO_HPP

#include "kernel/pre_emphasis_filter.hpp"

#endif // HIP_TENSOR_AUDIO_HPP
78 changes: 78 additions & 0 deletions src/modules/hip/kernel/pre_emphasis_filter.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#include <hip/hip_runtime.h>
#include "rpp_hip_common.hpp"

__device__ void pre_emphasis_filter_hip_compute(d_float8 *src1_f8, d_float8 *src2_f8, d_float8 *dst_f8, float4 *coeff_f4)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change device helpers to __device__ __forceinline__

{
dst_f8->f4[0] = src1_f8->f4[0] - *coeff_f4 * src2_f8->f4[0];
dst_f8->f4[1] = src1_f8->f4[1] - *coeff_f4 * src2_f8->f4[1];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix double space, and add parentheses around (*coeff_f4 * src2_f8->f4[0]) for clarity.

}

__global__ void pre_emphasis_filter_tensor(float *srcPtr,
uint2 srcStridesNH,
float *dstPtr,
uint2 dstStridesNH,
RpptImagePatchPtr srcDims,
float *coeffTensor,
RpptAudioBorderType borderType)
{
int id_x = (hipBlockIdx_x * hipBlockDim_x + hipThreadIdx_x) * 8 + 1;
int id_y = hipBlockIdx_y * hipBlockDim_y + hipThreadIdx_y;
int id_z = hipBlockIdx_z * hipBlockDim_z + hipThreadIdx_z;

if ((id_x >= srcDims[id_z].width) || (id_y >= srcDims[id_z].height))
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove braces since it has only 1 line inside if
Ensure to follow this practice of not having braces for 1 line if conditions, loops etc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return;
}

uint srcIdx = (id_z * srcStridesNH.x) + (id_y * srcStridesNH.y) + id_x;
uint dstIdx = (id_z * dstStridesNH.x) + (id_y * dstStridesNH.y) + id_x;

float4 coeff_f4 = (float4)coeffTensor[id_z];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all recent HIP kernels, we are using static_cast instead of c style casting
So please change this to

    float4 coeff_f4 = static_cast<float4>(coeffTensor[id_z]);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

d_float8 src1_f8, src2_f8, dst_f8;
rpp_hip_load8_and_unpack_to_float8(srcPtr + srcIdx, &src1_f8);
rpp_hip_load8_and_unpack_to_float8(srcPtr + srcIdx - 1, &src2_f8);
pre_emphasis_filter_hip_compute(&src1_f8, &src2_f8, &dst_f8, &coeff_f4);
rpp_hip_pack_float8_and_store8(dstPtr + dstIdx, &dst_f8);
}

RppStatus hip_exec_pre_emphasis_filter_tensor(Rpp32f *srcPtr,
RpptDescPtr srcDescPtr,
Rpp32f *dstPtr,
RpptDescPtr dstDescPtr,
RpptImagePatchPtr srcDims,
RpptAudioBorderType borderType,
rpp::Handle& handle)
{
int globalThreads_x = (dstDescPtr->w + 7) >> 3;
int globalThreads_y = dstDescPtr->h;
int globalThreads_z = handle.GetBatchSize();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use dstDescPtr->n instead of handle.GetBatchSize() in all the new HIP kernels we are going to add

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


float *coeff = handle.GetInitHandle()->mem.mgpu.floatArr[0].floatmem;

for(int i = 0; i < srcDescPtr->n; i++)
{
int id_x = i * srcDescPtr->strides.nStride;
if(borderType == RpptAudioBorderType::ZERO)
dstPtr[id_x] = srcPtr[id_x];
else
{
float border = (borderType == RpptAudioBorderType::CLAMP) ? srcPtr[id_x] : srcPtr[id_x + 1];
dstPtr[id_x] = srcPtr[id_x] - coeff[id_x] * border;
}
}

hipLaunchKernelGGL(pre_emphasis_filter_tensor,
dim3(ceil((float)globalThreads_x/LOCAL_THREADS_X), ceil((float)globalThreads_y/LOCAL_THREADS_Y), ceil((float)globalThreads_z/LOCAL_THREADS_Z)),
dim3(LOCAL_THREADS_X, LOCAL_THREADS_Y, LOCAL_THREADS_Z),
0,
handle.GetStream(),
srcPtr,
make_uint2(srcDescPtr->strides.nStride, srcDescPtr->strides.hStride),
dstPtr,
make_uint2(dstDescPtr->strides.nStride, dstDescPtr->strides.hStride),
srcDims,
coeff,
borderType);

return RPP_SUCCESS;
}
43 changes: 43 additions & 0 deletions src/modules/rppt_tensor_audio_augmentations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ SOFTWARE.
#include "rppt_tensor_audio_augmentations.h"
#include "cpu/host_tensor_audio_augmentations.hpp"

#ifdef HIP_COMPILE
#include "hip/hip_tensor_audio_augmentations.hpp"
#endif // HIP_COMPILE

/******************** non_silent_region_detection ********************/

RppStatus rppt_non_silent_region_detection_host(RppPtr_t srcPtr,
Expand Down Expand Up @@ -186,3 +190,42 @@ RppStatus rppt_resample_host(RppPtr_t srcPtr,
return RPP_ERROR_NOT_IMPLEMENTED;
}
}

/********************************************************************************************************************/
/*********************************************** RPP_GPU_SUPPORT = ON ***********************************************/
/********************************************************************************************************************/

#ifdef GPU_SUPPORT

/******************** pre_emphasis_filter ********************/

RppStatus rppt_pre_emphasis_filter_gpu(RppPtr_t srcPtr,
RpptDescPtr srcDescPtr,
RppPtr_t dstPtr,
RpptDescPtr dstDescPtr,
RpptImagePatchPtr srcDims,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a problem with this PR
There is no argument of type RpptImagePatchPtr in declaration
https://github.com/sampath1117/rpp/blob/sr/pre_emphasis_filter_hip/include/rppt_tensor_audio_augmentations.h#L98

@HazarathKumarM i think this PR wont be able to build
Please confirm

Copy link
Author

@HazarathKumarM HazarathKumarM May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the commits didn't get pushed. I have now pushed all the commits.

Rpp32f *coeffTensor,
RpptAudioBorderType borderType,
rppHandle_t rppHandle)
{
#ifdef HIP_COMPILE
Rpp32u paramIndex = 0;
copy_param_float(coeffTensor, rpp::deref(rppHandle), paramIndex++);

if ((srcDescPtr->dataType == RpptDataType::F32) && (dstDescPtr->dataType == RpptDataType::F32))
{
hip_exec_pre_emphasis_filter_tensor(static_cast<Rpp32f*>(srcPtr),
srcDescPtr,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check numDims = 3 and other such restrictions in the API file for all these PRs

static_cast<Rpp32f*>(dstPtr),
dstDescPtr,
srcDims,
borderType,
rpp::deref(rppHandle));
}
return RPP_SUCCESS;
#elif defined(OCL_COMPILE)
return RPP_ERROR_NOT_IMPLEMENTED;
#endif // backend
}

#endif // GPU_SUPPORT

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also not able to see any test suite changes
@HazarathKumarM
Please recheck this PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now pushed the changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add blank line at EOF for all new files you are adding

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please end all files with one blank line