DGemm with offset #24
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect bounds-checking calculations for offset-access in SGEMM, DGEMM, SGEMV, and DGEMV by adjusting the checkIndex formulas to use the precise last-element index rather than a loose multiplication-based bound. It also adds parameterized tests to reproduce and verify the fix for each of these routines.
- Refined
checkIndexlogic inAbstractBLAS.javafor both single- and double-precision GEMM and GEMV - Added
testOffsetBoundsCheckingtests in the four corresponding test classes - Ensures no
IndexOutOfBoundsExceptionis thrown for valid offsets and matches results against the F2J reference implementation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| AbstractBLAS.java | Updated checkIndex formulas for SGEMM, DGEMM, SGEMV, DGEMV |
| SgemvTest.java | Added testOffsetBoundsChecking for SGEMV |
| DgemvTest.java | Added testOffsetBoundsChecking for DGEMV |
| SgemmTest.java | Added testOffsetBoundsChecking for SGEMM |
| DgemmTest.java | Added testOffsetBoundsChecking for DGEMM |
Comments suppressed due to low confidence (3)
blas/src/test/java/dev/ludovic/netlib/blas/SgemvTest.java:109
- The new test only covers the non-transposed ('N') case; consider adding a similar
testOffsetBoundsCheckinginvocation with trans="T" to ensure the offset logic works for the transpose branch as well.
blas.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, y, 0, 1);
blas/src/test/java/dev/ludovic/netlib/blas/DgemmTest.java:192
- [nitpick] For consistency with the other test classes, you might clarify this comment to read "// Test case that reproduces the original bounds checking issue for dgemm".
// Test case that reproduces the original bounds checking issue
blas/src/main/java/dev/ludovic/netlib/blas/AbstractBLAS.java:294
- [nitpick] Consider applying the same precise offset-bound check adjustments to the complex routines (cgemm/zgemm and cgemv/zgemv) so that all BLAS variants remain consistent.
checkIndex(offseta + (lsame("N", transa) ? (k - 1) * lda + (m - 1) : (m - 1) * lda + (k - 1)), a.length);
|
I'll need to update some dependencies to run CI though, let me do that first. |
|
@Quafadas I merged your changes with some minor modifications. Thanks for the contribution! Very happy to review some more if you are interested :) |
|
thanks for the review / merge / release :-) I’m afk for a couple weeks, but still intending to keep exploring netlib. I have the hope, that I will not find more things :-)! But if I do I’ll see if I can follow a similar pattern to the way this issue panned out. |
### What changes were proposed in this pull request? This PR aims to upgrade `dev.ludovic.netlib` to 3.0.4 which is the first version tested with **Java 21 and 24** officially. ### Why are the changes needed? To bring the latest bug fixes. - https://github.com/luhenry/netlib/releases/tag/v3.0.4 - luhenry/netlib#24 - luhenry/netlib#25 (Temurin Java 21 and 24) ### Does this PR introduce _any_ user-facing change? No behavior change. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51945 from dongjoon-hyun/SPARK-53220. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Fixes #23
Full disclosure: I delegated this to copilot.
I'm not familiar with the netlib codebase, so I can't say whether or not this is in the right style, but the fix looks superficially the way I would have expected. Copilot added and ran extra tests, which reproduce and fix the case I reported.
Copilot also claimed to have checked for other regressions.
I'm hopeful, that the CI will tell us if anything has regressed.
I do note however, that this makes the offset checks strictly looser.
Loosening the offset conditions in this manner, would appear to open up a clutch of new code paths, which are not tested by this PR beyond this single naive test. This change may therefore be risky in terms of a preference to throw an error in preference to producing incorrect numbers. Feedback welcomed.