Fix - Address RPP -Wall warnings (Phase 1)#693
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces -Wall warning noise across RPP (Phase 1) while including a couple of correctness fixes in tensor CPU kernels and adding -Wall to the global CMake build flags.
Changes:
- Enable
-Wallglobally and temporarily suppress select warning categories to keep build logs usable during cleanup phases. - Fix several warning sources (unused functions/vars, missing braces) and add
nodiscardHIP runtime status checks inrandom_erase. - Fix two correctness bugs: uninitialized
locusage injitterand incorrectsumcomputation intensor_mean.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/tensor/hip/kernel/random_erase.cpp | Wrap HIP runtime calls with CHECK_RETURN_STATUS to satisfy nodiscard and improve error checking. |
| src/modules/tensor/cpu/kernel/tensor_mean.cpp | Fix sum initialization/aggregation for 3-channel NCHW i8 mean computation. |
| src/modules/tensor/cpu/kernel/jitter.cpp | Move compute_jitter_src_loc(...) before first use of loc to fix use-before-init. |
| src/modules/handle_hip.cpp | Initialize hipCtx_t ctx to suppress uninitialized warnings; minor formatting. |
| src/include/common/hip/rpp_hip_roi_conversion.hpp | Suppress unused static helpers via [[maybe_unused]]. |
| src/include/common/hip/rpp_hip_load_store.hpp | Add extra braces in aggregate init to address -Wmissing-braces. |
| src/include/common/cpu/rpp_cpu_simd_load_store.hpp | Remove unused variables to address -Wunused-*. |
| CMakeLists.txt | Add -Wall and suppress a subset of known-warning categories pending later cleanup phases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| compute_jitter_src_loc(&xorwowState, dstLocRow, vectorLoopCount, kernelSize, heightLimit, roi.xywhROI.roiWidth, srcDescPtr->strides.hStride, bound, layoutParams.bufferMultiplier, loc); | ||
| for(int cnt = 0; cnt < vectorIncrementPerChannel; cnt++) | ||
| { | ||
| srcPtrTemp_ps[cnt] = (Rpp16f)srcPtrChannel[loc + cnt]; | ||
| } |
There was a problem hiding this comment.
In this AVX2 NHWC->NHWC path, loc is computed for a single output pixel, but the subsequent loop reads 8 consecutive values starting at loc and later stores 8 values to dstPtrTemp[...] while the pointer is advanced by only 3 elements (one pixel). This will overlap writes and can go out-of-bounds, producing corrupted output and potential memory errors. Consider rewriting this block to either process 8 pixels per iteration (with corresponding 24-value RGB stores and pointer increments) or fall back to the scalar 3-channel copy per pixel.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #693 +/- ##
===========================================
- Coverage 92.45% 92.44% -0.01%
===========================================
Files 215 215
Lines 94897 94895 -2
===========================================
- Hits 87729 87719 -10
- Misses 7168 7176 +8
🚀 New features to boost your workflow:
|
|
@zacharyvincze code coverage seems to drop 8%. Can you please take a look? |
Latest ci run seems to not have this issue. We should be good |
Description
This PR handles some low-hanging fruit warnings in RPP which show up when
-Wallis enabled. Phase 1 aims to significantly reduce the number of warnings while keeping the PR diff low. Some bug fixes are included as part of the warning handling as well.Technical Details
Warning Fixes
rpp_hip_roi_conversion.hpphave been suppressed with[[maybe_unused]]. This suppression must be removed whenever these helpers are used in the codebase again. (A TODO was created here for easier tracking)rpp_hip_load_store.hppfunctions which is a hot path in the codebase.rpp_cpu_simd_load_store.hpp.nodiscardinrandom_erase.cpp.Bug Fixes
locinjitter.cppwas being used before initialization. Moved its calculation above the for loop which uses it.sumwas incorrectly calculated intensor_mean.cpp. It's initialization used itself (which is an uninitialized variable up to that point) instead ofsumRdefined above.Build System Changes
-Wallto the root CMake file. Categories which have not been fully handled will remain suppressed as to not pollute the build logs while warnings are being handled.Results
Note that many of these warnings may be duplicate lines from headers that have been included in multiple source files.
Before Phase 1 Fixes
After Phase 1 Fixes