Repository navigation
fix(optimize-memrefs): never share a slot with a host-accessed buffer - #1389
pschlan-amd wants to merge 1 commit into
Conversation
Op order is stream order only for device ops; a memref.store runs as soon as the host reaches it. In the GQA preamble the host-stored total_sequence_length scalar reused the ReduceSum(mask) buffer, which hip.sub still had to read, so seqlens_k intermittently came out as Shape(mask)[1] - 1 and attention scanned the whole static cache. Buffers with a memref.load / memref.store on any alias now keep their own allocation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for opening a PR! This project follows LLVM's incremental-development and AI-tool-use Before requesting review, please check that:
Reviewers are assigned through |
|
/run-test test_mode=performance models=all |
| } | ||
|
|
||
| /// Returns true if the host loads from or stores to any alias of \p alloc. | ||
| static bool isHostAccessed(Value alloc, |
There was a problem hiding this comment.
This is a generic pass, so we should not put any host- or device-specific logic in it. I made a similar comment on another PR (PR #1305), and we seem to be replicating the same solution in this PR. We need a backend-specific (CPU, GPU, or NPU) pass that runs late in the compiler flow. Please see the follow-up comment in the following PR.
|
Didn't realize there's already work in other PRs on this issue, hence closing here. |
L2 Accuracy Results (EP vs CPU)
Threshold: 0.01 | Run: 8160 - Commit: |
Binary sizesBaseline: run 40994390384 -
MorphiZen EP Performance Results
EPContext Export Performance
EPContext Import Performance
OGA Benchmark Results
OGA Wheel Smoke (Python benchmark_e2e.py)
VLM Benchmark Results
TTFT / TPS vs mainBaseline: run 40994390384 -
Run: 8160 - Commit: |
fix(optimize-memrefs): never share a slot with a host-accessed buffer
Problem
hip-optimize-memrefsreuses a dead buffer for a later allocation, and decides "dead" from op order in the block. Op order is stream order only for device ops. Amemref.storeormemref.loadruns on the host as soon as the host reaches it, while device ops issued earlier may still be queued.The standard onnxruntime-genai GQA preamble triggers this:
%sumis dead afterhip.subin op order, so%totalgot%sum's slot through amemref.reinterpret_cast.hip-materialize-host-scalarsthen moved that shared buffer into host-mapped scratch. Whichever side wrote last won:reduce_sumandsub.hip.subreadsShape(mask)[1], soseqlens_kbecomesmax_length - 1. Attention then runs over the whole static KV buffer, including the unwritten rows. The output is silently wrong, with no error.reduce_sum.total_sequence_lengthtakes the mask sum instead of the buffer length. Most fused GQA paths don't read it, so this usually stays hidden.The failure depends on timing. In a one-layer GQA replica (one prefill step, then decode steps, against an fp32 reference) it hit the first decode step in roughly 1 of 5 runs. The wrong output exactly matched attention over all 4096 buffer rows, and the device
seqlens_kread back as 4095 instead of 1536. EnablingHIPDNN_EP_DEBUG=1, or serialising withAMD_SERIALIZE_KERNEL=3 AMD_SERIALIZE_COPY=3, hid it.Fix
An allocation with a
memref.loadormemref.storeon any alias (as resolved byBufferViewFlowAnalysis) is left out of slot assignment. It neither reuses a slot nor provides one, and keeps its own allocation. Such buffers are the small shape and scalar staging buffers thathip-materialize-host-scalarslater places in host scratch, where each already gets its own offset. The memory cost is a few bytes per buffer.Device-only buffers are reused exactly as before.
Tests
@no_reuse_host_storeintest/lit/Dialect/hip-optimize-memrefs.mlirmirrors the preamble above. It checks that%totalkeeps its ownmemref.allocand that noreinterpret_castonto%sumappears.check-hip-mlir-lit), 663 passed and 9 unsupported.Impact
Numerics are unchanged whenever the race doesn't fire. Generated code changes only in that host-accessed allocations no longer alias device buffers. I verified the exact pattern only in the replica's IR. Because it comes from the standard genai preamble (
seqlens_k = ReduceSum(mask) - 1,total_sequence_length = Shape(mask)[1]), other GQA models are likely affected too, depending on how the allocations line up after CSE.Analyzed and fixed using AI assistance (Cursor / Claude Opus 5.5).