Skip to content

Test suite - Add QA pass/fail tests for F32 bit depth #549

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

Merged
merged 13 commits into from
Jun 3, 2025

Conversation

r-abishek
Copy link
Member

The PR adds internal QA testing support for F32 bit depth for next set of functions.

  • Add range checks rpp_pixel_check_0to1 to ensure F32 bit depth QA test pass.
  • Add necessary bin files for reference output.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces additional QA tests for F32 bit depth processing along with range‐check validations for tensor functions. Key changes include:

  • Adding new validations for data type and layout consistency before processing in several host and GPU kernels.
  • Inserting boundary checks for F32 values (using rpp_pixel_check_0to1) across multiple tensor processing functions.
  • Updating API definitions and simd load/store helper functions to support the new checks.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

File Description
src/modules/tensor/rppt_tensor_geometric_augmentations.cpp Added validations for matching data types and valid layouts prior to crop/patch operations.
src/include/common/cpu/rpp_cpu_simd_load_store.hpp Updated rpp_pixel_check_0to1 functions to modify vectors in place rather than returning clamped values.
src/modules/tensor/cpu/kernel/contrast.cpp Inserted boundary checks for F32 values to ensure pixel intensity remains in the expected [0, 255] range.
Comments suppressed due to low confidence (2)

src/include/common/cpu/rpp_cpu_simd_load_store.hpp:275

  • The function signature of rpp_pixel_check_0to1 has been changed to modify the vector in place rather than returning a value; please ensure that all call sites have been updated accordingly.
inline __m256 rpp_pixel_check_0to1_avx(__m256 p)

src/modules/tensor/cpu/kernel/contrast.cpp:310

  • [nitpick] The addition of boundary checks for F32 pixels ensures values remain within acceptable limits; verify that similar checks in other kernels are consistent and do not introduce performance regressions.
rpp_pixel_check_0to1(p, 3);

@kiritigowda kiritigowda self-assigned this Apr 29, 2025
@kiritigowda kiritigowda requested a review from Copilot April 29, 2025 21:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds F32 bit depth QA test support by introducing range validations and error checking both at the API level and inside the host/GPU kernels. Key changes include adding explicit error returns for mismatched dataType and unsupported layouts, inserting boundary check calls (rpp_pixel_check_0to1) after pixel processing, and updating SIMD load/store functions to work with the revised boundary enforcement.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/modules/tensor/rppt_tensor_geometric_augmentations.cpp Added validation checks on data types and layouts in crop functions.
src/modules/tensor/rppt_tensor_color_augmentations.cpp Introduced similar dataType/layout constraints and boundary checks.
src/modules/tensor/cpu/kernel/threshold.cpp Replaced legacy rpp_pixel_check_0to1_avx calls with the updated rpp_pixel_check_0to1 function for F32.
src/include/common/cpu/rpp_cpu_simd_load_store.hpp Updated the rpp_pixel_check_0to1 functions’ signatures to operate on vector arrays and return void.
... Several kernel files (phase.cpp, brightness.cpp, etc.) have analogous boundary check insertions and API updates.
Comments suppressed due to low confidence (1)

src/modules/tensor/cpu/kernel/threshold.cpp:42

  • The legacy rpp_pixel_check_0to1_avx call has been replaced by a new boundary check call; please verify that the new rpp_pixel_check_0to1 function correctly enforces the [0,1] range across all SIMD vector lengths.
p[0] = _mm256_blendv_ps(avx_p0, avx_p1, _mm256_and_ps(_mm256_and_ps(pChannelCheck[0], pChannelCheck[1]), pChannelCheck[2]));

Copy link
Contributor

@rrawther rrawther left a comment

Choose a reason for hiding this comment

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

Added few comments

Copy link

codecov bot commented May 6, 2025

Codecov Report

Attention: Patch coverage is 80.12422% with 32 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/include/common/cpu/rpp_cpu_simd_load_store.hpp 11.11% 32 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #549   +/-   ##
========================================
  Coverage    85.78%   85.78%           
========================================
  Files          185      185           
  Lines        78985    79060   +75     
========================================
+ Hits         67751    67819   +68     
- Misses       11234    11241    +7     
Files with missing lines Coverage Δ
src/modules/tensor/cpu/kernel/blend.cpp 100.00% <100.00%> (ø)
src/modules/tensor/cpu/kernel/brightness.cpp 94.31% <100.00%> (+0.05%) ⬆️
src/modules/tensor/cpu/kernel/color_cast.cpp 95.51% <100.00%> (+0.04%) ⬆️
src/modules/tensor/cpu/kernel/color_twist.cpp 89.18% <100.00%> (+0.07%) ⬆️
src/modules/tensor/cpu/kernel/contrast.cpp 100.00% <100.00%> (ø)
src/modules/tensor/cpu/kernel/exposure.cpp 100.00% <100.00%> (ø)
src/modules/tensor/cpu/kernel/magnitude.cpp 96.03% <100.00%> (+0.03%) ⬆️
src/modules/tensor/cpu/kernel/phase.cpp 96.95% <100.00%> (+0.02%) ⬆️
src/modules/tensor/cpu/kernel/threshold.cpp 95.62% <100.00%> (ø)
...modules/tensor/rppt_tensor_color_augmentations.cpp 98.36% <100.00%> (+0.05%) ⬆️
... and 2 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@rrawther rrawther left a comment

Choose a reason for hiding this comment

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

As we have discussed, please add the pixel check only in the functions where it is needed.

Copy link
Contributor

@rrawther rrawther left a comment

Choose a reason for hiding this comment

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

@r-abishek there was a comment which is not resolved

HazarathKumarM and others added 2 commits May 14, 2025 08:31
* Modified SIMD print functions to use union

* remove redundant unions in print functions

* removed pixel checks

* remove pixel check in threshold

* resolve review comments
@r-abishek
Copy link
Member Author

@rrawther All comments resolved now

@kiritigowda
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kiritigowda kiritigowda merged commit 04b0d62 into ROCm:develop Jun 3, 2025
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants