feat(rpp): added bilateral filter cpu kernel by isabellagoodwin970 · Pull Request #13786 · ROCm/rocm-libraries

isabellagoodwin970 · 2026-10-15T22:38:25Z

Motivation

Implements the HOST (CPU) kernel for rppt_bilateral_filter. The API, entry point and executor declaration came in as scaffolding on users/sruthi0107/rpp-bilateral-filter, where the CPU kernel was a stub that returned RPP_ERROR_NOT_IMPLEMENTED. This PR replaces that stub with a working scalar implementation that matches the golden model in bilateral_filter_ref.hpp.

Technical Details

Only src/modules/tensor/cpu/kernel/bilateral_filter.cpp changes. The header, entry point, executor declaration and tests are untouched.

Algorithm. For each image, channel and output pixel in the ROI, the kernel computes the weighted mean over a square K×K window (K ∈ {3, 5, 7, 9}):

w = exp(-(dy² + dx²) / 2σs²) · exp(-(v - v0)² / 2σr²)
dst = Σ w·v / Σ w

Spatial weights are precomputed once per image into the host scratch buffer (create_bilateral_spatial_kernel_host).
Range weights depend on the data type:
- U8 / I8: a 256-entry lookup table indexed by |v - v0|. Differences are always whole numbers in [0, 255], so the lookup is exact.
- F16 / F32: exp(-(v - v0)² · rangeMulFactor) is computed directly, with rangeMulFactor = 1 / (2σr²) precomputed per image. Float differences aren't whole numbers, so a table can't be indexed by them.
Pixels are accumulated in Rpp32f for every data type.
Store: U8/I8 round to nearest (nearbyintf) and then saturate. F16/F32 are not rounded and are clamped to [0, 1]. I8 adds +128 before saturate_pixel to offset that helper's −128 shift. σr is used in stored units, with no rescaling.
Per-image parameters: σs and σr are read as [batchCount].
Border: REPLICATE, clamped to the ROI, so no pixels outside the ROI are read.
Layouts: PLN1, PLN3, PKD3, PKD3→PLN3 and PLN3→PKD3, each addressed through its own src/dst strides.
Parallelism: one image per OpenMP thread, following gaussian_filter_host_tensor.

Not in this PR:

AVX2 vectorization. This is the plain scalar version; optimization (for example fast_exp_avx for the range term) is planned as a follow-up, with these tests as the safety net.
The HIP backend, which still returns RPP_ERROR_NOT_IMPLEMENTED from the scaffold.

Test Plan

Built the library with make rpp (-DBACKEND=HIP -DCMAKE_BUILD_TYPE=Release -DGPU_TARGETS=gfx1100) and installed it to a local prefix.
Copied the tests from users/sruthi0107/rpp-bilateral-filter-tests (golden model and gtest cases, on top of the gtest suite in feat(rpp): Add a GoogleTest correctness suite for RPP tensor operations #11802) into the working tree without committing them. Built rpp_tests against the local install and confirmed with ldd that it loads the new librpp.so.
Ran ./rpp_tests --gtest_filter='*BilateralFilter*/HOST_*'.

The test grid covers:

Data types: U8, I8, F16, F32
Layouts: PLN1, PLN3, PKD3, and both mixed layouts
ROI: full and partial
Sizes: 2x36x48, 2x36x55 (odd tail width) and 1x45x13 (narrower than one AVX2 vector)
Parameters: K = 3/5/7/9; σs = 0.25/1/5; and the two limiting cases, σr = 0.1 (identity) and σr = 10000 (reduces to a Gaussian)
Tolerances: 1 LSB for U8/I8, 1e-3 for F32, 1e-2 for F16

Test Result

All 711 HOST tests pass:

BilateralFilterTest.Correctness: 704/704
BilateralFilterNegativeTest.Negative: 7/7

HIP cases were not run; that backend is not implemented yet.

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.

therock-pr-bot · 2026-10-15T22:42:32Z

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🔎 pre-commit	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—
🤖 therock-pr-bot	✅ Pass	—

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

therock-pr-bot · 2026-10-15T22:42:36Z

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

python -m pip install pre-commit
pre-commit install
pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

therock-pr-bot · 2026-10-15T22:59:24Z

🎉 All checks passed! This PR is ready for review.

Copilot

🟡 Changes recommended

Unresolved bounds errors, single-channel handling, HIP parameter access, numerical edge cases, and unsupported-backend tests affect correctness and safety.

5 open issues

What changed in this PR

Adds a scalar CPU bilateral filter to RPP’s tensor image operations. The supplied diff also includes API scaffolding and correctness tests, beyond the kernel-only scope described in the PR.

Changes:

Implements per-image spatial and range weighting for four data types and planar/packed layouts.
Adds the public API, argument validation, and HOST dispatch; HIP remains unimplemented.
Adds an independent reference model and parameterized correctness and negative tests.

File	Description
projects/rpp/utilities/test/src/tests/image/filter/bilateral_filter_test.cpp	Adds correctness grids and invalid-argument tests.
projects/rpp/utilities/test/src/reference/bilateral_filter_ref.hpp	Adds a double-precision reference model.
projects/rpp/src/modules/tensor/rppt_tensor_filter_augmentations.cpp	Adds validation and backend dispatch.
projects/rpp/src/modules/tensor/cpu/kernel/bilateral_filter.cpp	Implements scalar HOST filtering and layout handling.
projects/rpp/src/include/tensor/host_tensor_executors.hpp	Declares the HOST executor.
projects/rpp/api/rppt_tensor_filter_augmentations.h	Declares and documents the public API.

_{Review effort: Balanced}

Declares rppt_bilateral_filter and wires its HOST path end to end, with the CPU kernel left as a stub (returns RPP_ERROR_NOT_IMPLEMENTED) to be implemented on top. - API: rppt_tensor_filter_augmentations.h. Per-image stdDevSpaceTensor (pixels) and stdDevRangeTensor (stored pixel units), kernelSize 3/5/7/9, REPLICATE border only, per-channel range distance. - Entry point: argument validation (kernelSize, datatypes, layouts, batch size, border type, stdDevs > 0) and U8/F16/F32/I8 HOST dispatch. HIP returns RPP_ERROR_NOT_IMPLEMENTED. - host_tensor_executors.hpp: bilateral_filter_host_tensor declaration. - cpu/kernel/bilateral_filter.cpp: stub with the spec in a comment and the four explicit instantiations.

Reference model and Correctness/Negative tests for the new rppt_bilateral_filter op, in the ROCm#11802 suite layout. - Golden reuses filter_common.hpp's window: square KxK, REPLICATE clamped to the ROI, per-channel range distance, per-image stdDevs, stdDevRangeTensor in the dtype's stored units. - Correctness: 704 cases per backend (gaussian_filter's grid) with identity (sr0p1) and gaussian-limit (sr10000) parameter corners. - Negative: even/out-of-range kernelSize, zero/negative stdDevs and a non-REPLICATE border must not return RPP_SUCCESS. Requires an RPP that declares rppt_bilateral_filter.

AryanSalmanpour

Reviewed the new HOST kernel at e71e99f6. I reimplemented the kernel's float arithmetic against the double reference and ran both over PKD3/PLN3/PLN1, partial ROIs, ROIs smaller than K, K in {3,5,7,9}, U8/I8/F32 and the degenerate-sigma corners: max deviation 0 LSB on U8/I8 and <=1.2e-7 on F32. I found no correctness defect — the I8 +128 / saturate_pixel pairing, the colOffsets bounds and the 256-entry LUT indexing are all exactly in range, and weightSum >= 1 always.

Two defensive/efficiency points below. Copilot's five earlier findings all look resolved by this rewrite (I re-checked the tiny-sigma NaN case numerically), so nothing here repeats them. Separately, this kernel has no intra-image parallelism and no AVX2 path unlike every sibling filter — that needs a coordinated change across the signature and call site, so I have raised it outside the diff rather than as a suggestion.

…lter

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot

🟡 Changes recommended

Saturating the range multiplier produces incorrect filtering for valid tiny-sigma F32 inputs.

1 open issue

Avoid reciprocal saturation for low-magnitude F32 sigma · New

_{Review effort: Balanced}

sruthi0107

All tests pass and the implementation looks good to me

assistant-librarian · 2026-10-30T17:14:07Z

TheRock Submodule Bump Activity

Newest first

2026-10-30 — Included in TheRock via ROCm/TheRock#9626.

Resolves conflicts with rppt_bilateral_filter (#13786), which was appended at the same place in rppt_tensor_filter_augmentations.h/.cpp. Both ops are kept, bilateral first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions Bot added the project: rpp label Oct 15, 2026

assistant-librarian Bot added the organization: ROCm label Oct 15, 2026

isabellagoodwin970 requested a review from kiritigowda October 16, 2026 17:10

isabellagoodwin970 changed the title ~~Users/isabellagoodwin970/rpp bilateral filter~~ feat(rpp): added bilateral filter cpu kernel Oct 21, 2026

isabellagoodwin970 marked this pull request as ready for review October 21, 2026 21:08

isabellagoodwin970 requested a review from a team as a code owner October 21, 2026 21:08

AryanSalmanpour requested a balanced review from Copilot October 22, 2026 03:26

Copilot started reviewing on behalf of AryanSalmanpour October 22, 2026 03:27

View session

Copilot AI reviewed Oct 22, 2026

View reviewed changes

sruthi0107 and others added 5 commits October 23, 2026 14:32

feat(rpp): Implement bilateral_filter HOST kernel

4e3c4a6

pre-commit check changes

a4f07fa

fix(rpp): address Copilot review on bilateral_filter

e71e99f

isabellagoodwin970 force-pushed the users/isabellagoodwin970/rpp-bilateral-filter branch from ba2b96c to e71e99f Compare October 23, 2026 22:21

AryanSalmanpour reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rpp/src/modules/tensor/cpu/kernel/bilateral_filter.cpp Resolved

Comment thread projects/rpp/src/modules/tensor/cpu/kernel/bilateral_filter.cpp Resolved

isabellagoodwin970 and others added 2 commits October 27, 2026 13:23

Apply suggestions from review

193bd94

Merge branch 'develop' into users/isabellagoodwin970/rpp-bilateral-fi…

5e6bbd8

…lter

isabellagoodwin970 requested a balanced review from Copilot October 27, 2026 23:15

Copilot AI reviewed Oct 27, 2026

LakshmiKumar23 requested a balanced review from Copilot October 27, 2026 23:17

Copilot started reviewing on behalf of LakshmiKumar23 October 27, 2026 23:18

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rpp/src/modules/tensor/cpu/kernel/bilateral_filter.cpp Resolved

sruthi0107 mentioned this pull request Oct 28, 2026

[Issue][RPP]: Tensor ops don't check that srcDesc->c == dstDesc->c: 3→1 writes past the destination, 1→3 leaves 2/3 of it unwritten #14582

Open

sruthi0107 self-requested a review October 28, 2026 18:03

sruthi0107 approved these changes Oct 28, 2026

View reviewed changes

AryanSalmanpour approved these changes Oct 28, 2026

View reviewed changes

sruthi0107 merged commit f1677ae into ROCm:develop Oct 29, 2026
86 checks passed

Conversation

isabellagoodwin970 commented Oct 15, 2026

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Checks Passed — Ready for Review

Uh oh!

therock-pr-bot Bot commented Oct 15, 2026

Pre-commit check failed

Uh oh!

therock-pr-bot Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AryanSalmanpour left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

sruthi0107 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

assistant-librarian Bot commented Oct 30, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

therock-pr-bot Bot commented Oct 15, 2026 •

edited

Loading

therock-pr-bot Bot commented Oct 15, 2026 •

edited

Loading