Repository navigation
testintel: fix severe issue in sellmvt_device (created by kliegeoi (@kliegeois)) - #13339
Conversation
Candidate cand:430c77788a2402dc4dfa26da9c0a1b5b at fb79226.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Also, |
With 32-bit indices, row = sell_slice_size * sliceid + tidx was computed in uint32 and stored in int32. Near m = 2^31 it wrapped negative and passed the row < m check, so padding slices read past the end of sell_slice_offsets and sellmvn_device could write y at a negative index. The last real slice wraps too when m is not a multiple of the slice size, so the row index is now computed in int64_t in all four SELL kernels. Add the changelog entry and the copyright year, and drop the test added earlier in this PR: padding threads have start == end == 0, so it passed with or without the fix.
|
@ntrost57 Thanks. Addressed in 6f18a21:
I checked it on an MI325X with a standalone kernel that calls
All 2^31 - 1 rows of |
|
TestIntel PR Steward: Not merging yet. ntrost57's review comments were already addressed in 6f18a21: the changelog entry, the copyright year, GitHub still reports the PR as
A human needs to re-approve on the current head and clear the team review. After that the PR is ready to merge, and I'll merge it on the next poll once GitHub reports it mergeable. |
|
TestIntel PR Steward: ntrost57 approved the current head (316636e), and there are no failing or pending checks, conflicts or change requests. GitHub still reports the PR as |
|
TestIntel PR Steward: ntrost57 approved the current head (316636e) and no checks are failing, but GitHub now reports merge conflicts with |
…da9c0a1b5b-2 Resolve the conflict in projects/rocsparse/CHANGELOG.md by keeping the Resolved issues entry that develop added and putting this PR's SELL SpMV entry after it.
|
TestIntel PR Steward: The branch now has a new head (0a27efc) and GitHub no longer reports a conflict with
Once the checks pass and the head is re-approved, I'll merge on the next poll if GitHub reports the PR as mergeable. |
|
Merged the latest @kliegeois @ntrost57 one blocker remains that I can't clear from here: the required |
|
TestIntel PR Steward: ntrost57 and kliegeois have approved the current head (0a27efc), no checks are failing or pending, and there are no conflicts or change requests. The PR is ready except for one blocker: GitHub still reports it as |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #13339 +/- ##
===========================================
- Coverage 75.93% 75.93% -0.00%
===========================================
Files 2873 2873
Lines 477006 477006
Branches 71572 71572
===========================================
- Hits 362207 362186 -21
- Misses 90102 90135 +33
+ Partials 24697 24685 -12
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
|
TestIntel PR Steward: The only failing check on the current head (2fa5511) is |
|
TestIntel PR Steward: The check |
|
TestIntel PR Steward: The current head (2fa5511) is approved by kliegeois and ntrost57, but GitHub again reports merge conflicts with |
|
@kliegeois @ntrost57 this is ready except for Math CI. Math CI passed on 2fa5511, but after the Oct 16 merge from Could one of you re-run Math CI for PR-13339 or ask a gardener to push it through, and then merge? Neither of my accounts has write access to this repository, so I can't merge it myself. |
|
TestIntel PR Steward: jsandham has approved the current head (3607aaf), and no checks are failing or pending. GitHub still reports the PR as |
|
TestIntel PR Steward: The check |
|
TestIntel PR Steward: The check |
|
@eidenyoshida could you advise why the math CI didn't run? |
TheRock Submodule Bump ActivityNewest first
|
…OCM-33114) (#13879) JIRA ID : ROCM-33114. ## Summary - Add device unit tests for ELL triangular solve and SELL SpMV. The YAML coverage misses the slice-size boundaries and a partial last slice. - ELL solve covers a lower non-unit diagonal, a unit diagonal whose stored diagonal must be ignored, and a zero pivot. The zero-pivot output is preset to a sentinel and its status must match the reported position. Those cases are built only when the library has ELL TRSV. - SELL SpMV covers the small-slice kernel (slice 32 and the 128 limit) and the large-slice kernel (slice 129, and slice 300 for two passes per slice), in both directions, each with a partial last slice; m = 600 also leaves a surplus block in the launch grid. It also covers more than 8 entries per row in the small-slice kernel, slice size 1 (the CSR fallback), non-zero beta in both directions, and a transpose case with n > m. The beta = 0 cases start from a non-zero y. - y is allocated with a sentinel tail, so a write past row m fails the test. Out-of-bounds reads (for example by padding threads in the transpose kernels) are not detectable by value checks and need a sanitizer. - Comparisons are exact: every value is a small integer or dyadic fraction and partial sums stay below 2^24. - This does not change `sellmv_device.h`. It is based on develop after #13339, which guards the padding-thread read of `x` in the small-slice transpose kernel. ## Test plan - [x] Built `rocsparse-unit-test-device` for gfx1201; `EllSv.*:SellMv.*` and the full binary pass. - [x] Removing `row < m` from the non-transpose large-slice guard makes `SellMv.none_slice_size_boundaries` fail. - [ ] CI
This PR was created by kliegeoi (@kliegeois) via Test Gap Resolver.
Motivation
The SELL SpMV kernels compute a thread's row as
sell_slice_size * sliceid + tidx. With 32-bit indices (J = int32_t) this is evaluated inuint32and stored inint32, so near m = 2^31 it wraps negative and passes therow < mcheck. That happens in the padding slices of the launch grid (blocks_x * blocks_y >= nslices) and in the last real slice whenever m is not a multiple of the slice size. Padding slices then read past the end ofsell_slice_offsets,sellmvn_devicecan writeyat a negative index, andsellmvt_devicereadsxat a negative index.Separately,
sellmvt_devicereadx[row]for every padding thread (row >= m), past the end ofx.Technical Details
rowinint64_tinsellmvn_device,sellmvn_large_slice_device,sellmvt_deviceandsellmvt_large_slice_device.sellmvt_devicereadsx[row]only whenrow < m, assellmvt_large_slice_devicealready did.Test Plan
No client test is added. Padding threads have
start == end == 0, so the strayxread never changes the output, and reaching the overflow needs m close to 2^31 (8 GiB per single-precision vector plus a host reference over 2^31 rows). Instead:rocsparse_sellmv_impl.cppfor gfx942 with the ROCm 10 compiler (clang 23).sellmvn_deviceandsellmvn_large_slice_devicewith the launch configuration ofsellmv_dispatch, on an empty matrix with 32-bit indices and m = 2^31 - 1, with a 2^31-element guard in front ofy, once with the old header and once with the new one.Test Result
rocsparse_sellmv_impl.cppcompiles without warnings.yfor slice sizes 3, 5, 7, 64, 100, 127 and 200 (between 6,424 and 423,664 writes per size). Slice size 2, which has no padding slices and no wrap, has none.yfor any slice size, and all 2^31 - 1 rows ofyare written.Submission Checklist
JIRA ID : AISPARSE-806