Skip to content

Commit 97f1cc9

Browse files
committed
Fix silently incorrect CPU matmul for a stride-zero broadcast batch
The baseline CPU backend folded a batched matmul with a broadcast lhs into a single gemm over (1, m, b * n, k). That fold is not valid: rhs is batch-major, so its batches are not adjacent columns, and dst_rs stays n while each merged row is b * n wide. The gemm wrote over itself, leaving only (m - 1) * n + b * n elements written and the rest at their zero-initialised value. Drop the fold. The per-batch loop below already handles a_skip == 0 by feeding the same lhs to every step, which is the correct broadcast semantics. Fixes #3744
1 parent ddf1b87 commit 97f1cc9

2 files changed

Lines changed: 37 additions & 2 deletions

File tree

candle-core/src/cpu_backend/mod.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1485,12 +1485,17 @@ impl Map2 for MatMul {
14851485
Parallelism::None
14861486
};
14871487
let (b, m, n, k) = if b_skip == 0 && a_skip == m * k {
1488+
// A batch-invariant rhs lets the batches of lhs stack into the rows of a single
1489+
// (b * m, k) matrix, which the destination already matches row for row.
14881490
// a_skip and c_skip should be updated but step is always 0 so
14891491
// it wouldn't matter.
14901492
(1, b * m, n, k)
1491-
} else if a_skip == 0 && b_skip == n * k {
1492-
(1, m, b * n, k)
14931493
} else {
1494+
// There is deliberately no mirrored fold for a batch-invariant lhs. Stacking the
1495+
// batches of rhs into the columns of a (k, b * n) matrix would need those columns
1496+
// to be adjacent, but rhs is batch-major, and the destination rows would still be
1497+
// n apart while each merged row is b * n wide. Both make the merged gemm write
1498+
// over itself, so a stride-zero batch on lhs goes through the loop below.
14941499
(b, m, n, k)
14951500
};
14961501
for step in 0..b {

candle-core/tests/matmul_tests.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,30 @@ fn broadcast_matmul(device: &Device) -> Result<()> {
8282
Ok(())
8383
}
8484

85+
// A stride-zero batch dim on the lhs, as produced by `broadcast_as`, must give the same
86+
// result as materializing that batch. The batches of rhs cannot be folded into the columns
87+
// of one matmul the way a batch-invariant rhs folds into the rows, so this exercises the
88+
// per-batch loop.
89+
fn broadcast_matmul_stride_zero_lhs(device: &Device) -> Result<()> {
90+
for (b, m, n, k) in [(32, 32, 32, 32), (3, 1, 2, 5), (4, 2, 3, 1), (2, 5, 1, 3)] {
91+
let lhs = Tensor::randn(0f32, 1f32, (1, m, k), device)?;
92+
let rhs = Tensor::randn(0f32, 1f32, (b, k, n), device)?;
93+
94+
let out = lhs.broadcast_as((b, m, k))?.matmul(&rhs)?;
95+
assert_eq!(out.dims(), &[b, m, n]);
96+
97+
// Every batch is the same lhs against that batch of rhs.
98+
let lhs = lhs.i(0)?;
99+
for idx in 0..b {
100+
let diff = (out.i(idx)? - lhs.matmul(&rhs.i(idx)?)?)?
101+
.sqr()?
102+
.sum_all()?;
103+
assert!(diff.to_vec0::<f32>()? < 1e-6, "batch {idx} differs");
104+
}
105+
}
106+
Ok(())
107+
}
108+
85109
fn zero_matmul(device: &Device) -> Result<()> {
86110
let lhs = Tensor::zeros((2, 0), DType::F32, device)?;
87111
let rhs = Tensor::zeros((0, 3), DType::F32, device)?;
@@ -238,6 +262,12 @@ test_device!(
238262
broadcast_matmul_gpu,
239263
broadcast_matmul_metal
240264
);
265+
test_device!(
266+
broadcast_matmul_stride_zero_lhs,
267+
broadcast_matmul_stride_zero_lhs_cpu,
268+
broadcast_matmul_stride_zero_lhs_gpu,
269+
broadcast_matmul_stride_zero_lhs_metal
270+
);
241271
test_device!(
242272
zero_matmul,
243273
zero_matmul_cpu,

0 commit comments

Comments
 (0)