Numba: solve triangular C-contiguous B without copy - #2359
Numba: solve triangular C-contiguous B without copy#2359jessegrabowski wants to merge 10 commits into
Conversation
…rtrs Every graph-produced right-hand side is c-contiguous, so SolveTriangular always fell through to a transposing copy of B before calling trtrs; the flag-flipping trick that avoids the copy for A has no analog for B, since trtrs cannot consume a row-major rhs. trsm with side="R" solves the equivalent transposed system straight out of B's memory, cutting a (128, 100_000) solve from 39ms to 14ms. trans=2 keeps the trtrs path because op(A)^T is conj(A), which trsm has no mode for, and trsm reports no INFO, so a zero pivot has to be caught before the call to preserve the fill-with-NaN behavior. Closes pymc-devs#2358
|
We should at some point explore outputting in F order (or rather batched F order) directly. Question, is this the same speed for C arrays as the "regular solver" is for F arrays. Tbis question isn't a blocker, because there will always be arrays we don't control (and right now we always emit C anyway) |
|
I think you're asking if we could just drop trtrs entirely? The answer is yes. The layout of A basically doesn't matter because we already do transpose tricks on that. There's a difference based on layout of B. But they seem to boil down to the same kernel when both are comparable. Timings: |
|
So let's go with that and drop the other. It sounds like trsm is what trtrs uses internally anyway. The zero check is also something we may want to over optimize later down the road when we know that it can't have one by construction (if that's a thing). It also exposes a scalar alpha parameter. Not sure where that shows up in our graphs but another potential benefit. |
| self, lower: bool, trans: int, A_order: Literal["C", "F"] | ||
| ): | ||
| # test_solve_triangular covers trans=0 through the graph, which is all the op ever passes. | ||
| # The overload is called directly to reach trans=1 and 2, with a complex dtype so that a |
There was a problem hiding this comment.
why add the test now? presumably if we find a need later then we can add such tests? for now you could just hardcode trans and mention in comment that there's no test for other cases?
Reference LAPACK's trtrs is a zero-pivot scan followed by a side="L" trsm, so it buys nothing over calling trsm directly once we do that scan ourselves, which the side="R" path already had to. Benchmarked equal within one sigma on an f-contiguous rhs across four shapes.
|
I removed trtrs. The pr message has been updated accordingly. |
| np.testing.assert_allclose(result, expected, rtol=rtol) | ||
|
|
||
| @pytest.mark.parametrize("b_shape", [(5, 3), (5,)], ids=["b_matrix", "b_vec"]) | ||
| def test_solve_triangular_singular_returns_nan(self, b_shape: tuple[int, ...]): |
There was a problem hiding this comment.
should be covered by a test using pytensor graphs, if there's none already
There was a problem hiding this comment.
Sure. This one does matter because trsm doesn't return info, so we're responsible for detecting failure now.
ricardoV94
left a comment
There was a problem hiding this comment.
Looks good just nit with tests
It was allocated per call, which is 19ns and a fifth of the runtime when the systems are small enough for the batch to be dominated by per-call overhead.
|
Ok if I squash merge? |
Every rhs a graph produces is c-contiguous, so SolveTriangular always fell through to a transposing copy of B before calling trtrs. It now calls trsm with the side picked from the rhs layout: side="R" solves the transposed system straight out of a c-contiguous B, side="L" takes an f-contiguous one. That takes a (128, 100_000) solve from 39ms to 14ms.
trtrs drops out of the path entirely. Reference LAPACK's trtrs is a zero-pivot scan followed by a side="L" trsm, and the side="R" path already needed that scan in our own code, so calling trsm directly costs nothing; benchmarked equal within one sigma on an f-contiguous rhs across four shapes.
A c-contiguous rhs is now consumed in place under overwrite_b, where it previously survived because trtrs copied it.
numba_xtrtrs in _LAPACK is unused after this and left in place; happy to drop it here if you'd rather.
The first two commits are unrelated: the numba linalg tests only pass under floatX=float32 with them.
Closes #2358