feat: naive attention kernel - #51
Conversation
f3d7997 to
bf39628Compare
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review
I traced through the full kernel stack state — the logic is correct:
- Dot product:
Q[row,:] . K[t,:]via a loop, scaled by1/sqrt(head_dim)✓ - Causal mask:
t > row → -1e30✓ - Softmax: numerically stable (max subtraction before exp), serial reductions by thread 0 ✓
- V accumulation:
O[row,t] = sum_j attn[j] * V[j,t]✓ - Stack is clean at exit ✓
Issues
1. Vendored binary — lib/Bitcode/libdevice.10.bc (484KB)
This is NVIDIA's libdevice checked directly into git (no LFS). Concerns:
- Repo bloat: Binary blobs in git history can't be garbage-collected.
- Licensing: libdevice is distributed under the NVIDIA EULA. Redistributing it may require attribution or may not be permitted.
- Alternative: Download from the CUDA toolkit at build time, use Git LFS, or locate it at the system CUDA install path (
/usr/local/cuda/nvvm/libdevice/libdevice.10.bc).
2. Hardcoded libdevice path via compile definition
target_compile_definitions(obj.MLIRConversionPasses PRIVATEWARPFORTH_LIBDEVICE_PATH="${WARPFORTH_LIBDEVICE_PATH}")The path is baked in at compile time. If the build directory moves or the binary is installed elsewhere, the pipeline will fail. Consider a fallback chain: env var → CUDA toolkit path → bundled path.
3. Variable shadowing in test assert (minor)
assertresult== [pytest.approx(v) forvinexpected]The loop variable v shadows the outer v numpy array (line 616). Harmless but a linter would flag it — rename to e or x.
Non-blocking observations
- Duplicate kernel source between
test/Pipeline/attention.forthand inline intest_kernels.py— could drift apart, but acceptable since the tests serve different purposes (FileCheck vs runtime). - Serial reductions (thread 0 loops) are correct for "naive" — parallel reductions would be the natural follow-up.
- FileCheck test is minimal (only checks
gpu.binary @warpforth_module) — reasonable as a pipeline smoke test.
Overall the kernel and test logic look solid. The main concern is the vendored libdevice binary.
Uh oh!
There was an error while loading. Please reload this page.
Summary
lib/Bitcode/and link it in the NVVM pipeline for math intrinsics supportCloses#44