Uh oh!
There was an error while loading. Please reload this page.
fix(executorch): support KV-cache aliased I/O in the TensorRT delegate - #4445
Conversation
52d316b to
c5ab1e4Comparec5ab1e4 to
40e0486CompareConarnar
commented
Aug 1, 2026
What changed vs the earlier (delegate-owned) versionThe earlier revision made the delegate own the KV cache: because ExecuTorch This revision makes the cache caller-owned, above the delegate. Export / lowering (new):
Runtime (changed):
|
40e0486 to
2312f42Comparenarendasan
commented
Aug 3, 2026
@cehongwang Please review usage of the aliased i/o feature |
| engine_node, | ||
| _split_binding_names(_get_str(engine_info, INPUT_BINDING_NAMES_IDX)), | ||
| ) | ||
| output_names = _split_binding_names( |
There was a problem hiding this comment.
line 223:
Only inputs need this. Outputs are also bound positionally by the runtime,
but they aregetitem(engine_node, idx)nodes whose index order equals the
engine output-binding order. ExecuTorch lowering can reorder delegate outputs
(arrange_graph_outputsmoves buffer-mutation outputs ahead of user
outputs), but a TensorRT delegate partition is a functional inference engine
with no mutation outputs, so that pass is a no-op here and the output order is
preserved. If a TRT partition ever produced mutation outputs, outputs would
need the same node-identity reordering as inputs.
We need to account for the output order, or otherwise there is a mismatch
There was a problem hiding this comment.
The engine appends the buffer mutation at the end of output, while the delegate prepends it
There was a problem hiding this comment.
Runtime walkthrough
Assume:
TRT inputs: [tokens, k_cache_in]
TRT outputs: [logits, k_cache_out]
ExecuTorch passes:
args[0] = tokens
args[1] = k_cache_in
args[2] = k_cache_mutation
args[3] = logits
After consuming the inputs, arg_idx == 2.
First output iteration
o = 0
name = output_binding_names[0] = logits
arg = args[2] = k_cache_mutation
The backend binds TensorRT's logits output to the cache-mutation output storage.
Second output iteration
o = 1
name = output_binding_names[1] = k_cache_out
out_arg = args[3] = logits
TensorRT correctly binds k_cache_out to k_cache_in for the in-place update, but the backend treats the logitsEValue as its mutation output slot. Its reflect copy therefore writes the cache result into the logits output.
The result is effectively:
| TensorRT output | Lands in |
|---|---|
logits | cache mutation slot |
| cache update | logits slot |
If shapes or capacities differ, execution may fail during resize/binding/enqueue. If they are compatible, it can run successfully while returning incorrect logits and corrupting the observable cache state.
The required fix is to reorder serialized output_binding_names into actual delegate-output order, analogous to _reorder_input_names_for_executorch.
There was a problem hiding this comment.
I was not able to reproduce this. Seems like arrange_graph_outputs reorders the submodule outputs, output_specs, and the parent getitems, but it doesn't touch the call node's meta["val"]. node.args is permuted by fusion (so _reorder_input_names_for_executorch is still needed), but outputs go through meta["val"], which arrange_graph_outputs leaves alone.
If meta["val"] is being rearranged somewhere that would be a problem, but the fix would be different from how _reorder_input_names_for_executorch handles it.
There was a problem hiding this comment.
Ok I spend a ton of time on this and it is quite surprising that the order was actually correct. But here is some findings:
_keep_mutated_buffers_above_delegate(exported_program)
https://github.com/Conarnar/TensorRT/blob/b9dbb306a4597eb4fb0c4ef0fdb546235d3924b1/py/torch_tensorrt/executorch/partitioner.py#L165
This function lifes mutated buffer to the executorch program level, therefore arrange_graph_outputs did not have any buffer_mutation, and therefore the order is perserved.
Uh oh!
There was an error while loading. Please reload this page.
cehongwang
commented
Aug 4, 2026
One testing gap worth closing before merge: there's currently no test anywhere that exercises a full That means the export side and the blob side are each tested in isolation and each is individually correct, while the bug lives in their composition. Could we get:
(2) is what RFC 0003 §7.4 asked for, and it would also cover the device-residency and reflect-path questions raised elsewhere in this review. Happy to gate it on GPU availability, but it should exist as a runnable target. |
cehongwang
commented
Aug 4, 2026
In The comment says output order is "stable by construction (getitem index order == engine output-binding order)." That was true when a TRT partition was purely functional. This PR introduces The fixture here has no mutation outputs, so it passes — but it asserts "preprocess must pass output names through unchanged," which is the behavior that needs to change. Could you update this test to cover the mutation case? Something like a fixture with one aliased output and one user output, asserting the serialized |
| engine->cached_input_sizes[i] = 1; | ||
| } | ||
| bind_ptr = engine->cached_input_ptrs[i]; | ||
| } else if (engine->unified_memory || is_cuda_accessible_ptr(et_in.const_data_ptr())) { |
There was a problem hiding this comment.
Aliased inputs can still take the host-staging path, which breaks caller-owned semantics.
If an aliased input's data pointer isn't CUDA-accessible, this falls through to the staging branch and binds engine->cached_input_ptrs[i] — a delegate-owned scratch buffer. The aliased output then binds to input_bind_ptrs[alias_in], i.e. that same scratch buffer, so the in-place KV update lands in delegate scratch rather than the caller's storage. On the next execute() the staging copy re-reads the caller's unchanged host buffer, so the update is silently lost. Recovery depends entirely on the reflect copy, which has its own problem (see the reflect comment).
There was a problem hiding this comment.
Can you clarify what RFC 0003 §6.4 (and §7.4) is? I was not able to find any references for that.
There was a problem hiding this comment.
Oh, here it is:
Require the caller's tensor itself to be device-resident. Aliasing only
works when the pointer bound to the input is the caller's real storage. Two
existing branches break that: the H2D fallback copies a host tensor into a
device staging buffer and binds that, and the zero-byte branch binds a
cached scratch allocation. Either would make the engine write the update
somewhere the caller never reads. For an alias-source input, gate on the
caller tensor being device/unified memory (is_cuda_accessible_ptr) with
non-zero size, and otherwise return Error::InvalidArgument ("aliased input
'%s' must be on GPU or unified memory") — i.e. reject before both the
staging and zero-byte paths, not only the H2D one.
| @@ -590,7 +724,7 @@ | |||
| const bool must_sync = output_staged_to_host || input_staged_from_host || !g_user_stream_set; | |||
There was a problem hiding this comment.
Do we need to account for aliase I/O? Is there a race possible?
There was a problem hiding this comment.
Yes, but only on the non-zero-copy reflect path: with a caller stream active and no end sync, a pending reflect into the delegate output could still be in flight when ExecuTorch's buffer-mutation copy_ reads it. Will handle it.
| // execute() can bind it to that input's device pointer (in-place). | ||
| // Non-aliased models have an empty header.aliased_io -> all -1, unchanged path. | ||
| handle->output_aliased_input_idx.assign(handle->num_outputs, -1); | ||
| for (const auto& ab : header.aliased_io) { |
There was a problem hiding this comment.
This doesn't cross-check the persisted alias map against the engine, and it accepts unknown kind values.
The Python runtime's _TRTEngine._reconcile_aliased_io treats getAliasedInputTensor as the source of truth for kv_cache_update aliases and preserves user ones as metadata-trusted. Here, a kind that is neither "kv_cache_update" nor "user" — a typo in the wire format, or a future kind written by a newer exporter — skips the shape check and gets registered as if it were a KV alias, which then binds two tensors to the same storage.
Could you mirror the Python behavior:
- Reject unknown kinds with
Error::InvalidProgram. - For
kv_cache_update, compare the persistedab.inputagainstengine->getAliasedInputTensor(ab.output.c_str())and error on disagreement. - Keep
useras metadata-trusted after the shape check (TRT can't see those aliases).
Related: the parser leaves ab.kind empty when the "kind" key is absent, while the Python side defaults to "kv_cache_update". Once unknown kinds are rejected, that mismatch turns an old blob into a hard failure — worth defaulting to "kv_cache_update" in the parser to match.
2312f42 to
b9dbb30Compare| new_mutation_outputs: List[torch.fx.Node] = [] | ||
| for oi, out_name in enumerate(out_names): | ||
| if out_name not in aliased_io: | ||
| continue |
There was a problem hiding this comment.
This'd better be warnings or errors
There was a problem hiding this comment.
Guessing you mean the two continues below.
Uh oh!
There was an error while loading. Please reload this page.
b9dbb30 to
8db164fCompare| buf_node = input_nodes[ii] | ||
| buf_fqn = inputs_to_buffers.get(getattr(buf_node, "name", None)) | ||
| if buf_fqn is None or buf_fqn in already_exposed: | ||
| continue |
There was a problem hiding this comment.
The previous comment no longer holds, and debugging can be painful if anything comes wrong in the future. Change the comment to something like
def_reorder_input_names_for_executorch(
edge_program: ExportedProgram, engine_node: Any, input_names: List[str]
) ->List[str]:
"""Reorder TRT binding names into executorch_call_delegate argument order. The runtime binds positionally (``execute()`` arg ``i`` -> input_binding_names ``[i]``), but ExecuTorch fusion may permute the delegate placeholders relative to the TRT-submodule order that produced ``input_binding_names``. The names can't be matched (TRT names are semantic, lowered placeholders are generic ``arg_N``), so recover the permutation by node identity: the engine node's first arg lists its input nodes in binding order, so sort the names by each node's slot among the graph placeholders (its runtime delegate-arg position). Only inputs need this. Outputs are also bound positionally, but they are ``getitem(engine_node, idx)`` nodes whose index order equals the engine output-binding order, and that order survives lowering -- though not because the partition is mutation-free. With aliased-I/O (KV-cache) support a TensorRT partition *does* produce mutation outputs, and ``arrange_graph_outputs`` does move buffer-mutation outputs ahead of user outputs. It stays a no-op here because ``_keep_mutated_buffers_above_delegate`` (``partitioner.py``) strips the ``delegation_tag`` from mutated buffer placeholders, so they stay out of the delegate's state dict and constants; ExecuTorch's ``_get_new_signature`` then records the mutation as a plain ``USER_OUTPUT`` rather than a ``BUFFER_MUTATION`` (it uses the latter only when the delegate itself consumes the buffer). The lowered submodule therefore has no mutation specs, so ``arrange_graph_outputs`` computes the identity permutation and the getitem indices still line up with the engine's output bindings. That guarantee is conditional, not structural. If a mutated buffer is ever tagged into a delegate, its spec becomes ``BUFFER_MUTATION``, the delegate's outputs are permuted, and they would need the same node-identity reordering as the inputs below. """| new_signature = ExportGraphSignature( | ||
| input_specs=list(sig.input_specs), output_specs=new_output_specs | ||
| ) | ||
| return ExportedProgram( |
There was a problem hiding this comment.
The reconstruction drops example_inputs and verifiers, so the pass isn't a drop-in replacement for its input.
ExportedProgram.init also takes example_inputs and verifiers; neither is carried over, so the returned program has them unset. No impact on the current to_edge path, which is presumably why it hasn't surfaced, but it bites in two places:
If the pass moves to the exported_program branch, the saved artifact silently loses its example inputs.
AOTI rejects such a program outright: RuntimeError: exported_program.example_inputs is required to be set in order for AOTInductor compilation. I hit this while testing whether the pass fixes AOTI and had to restore _example_inputs by hand before I could even reach the real failure.
Passing both through costs two keyword arguments.
There was a problem hiding this comment.
Fixed in the next push. Both example_inputs and verifiers are threaded
through now.
I should mention that the retrace=False path has a related gap:create_trt_exp_program constructs its ExportedProgram withoutexample_inputs either, so AOTI would refuse a program saved that way for the
same reason. That one predates this PR and isn't a dropped value. There's no
source program to carry from, though it does receive arg_inputs it could use.
Let me know if you want that fixed as well.
shoumikhin
left a comment
There was a problem hiding this comment.
On the output ordering question, it does not reproduce at this head and I think the author's reply is right. Running the real ExecuTorch to_edge_transform_and_lower over the head partitioner and preprocess gives delegate args [cache, tokens, logits, cache_out] against blob names [cache_in, tokens, logits, cache_out], with the parent mapping delegate output 1 to the buffer mutation. Deleting _keep_mutated_buffers_above_delegate reorders the subprogram to [cache_out, logits] while the blob stays [logits, cache_out], which is exactly the swap that was described, so the mechanism is real but the helper prevents it.
The comments below are the blockers and majors I found. On the other side, the device-residency rejection is correctly placed before both the host-staging and zero-byte paths, the inflight_pending drain gap I raised earlier is fixed, and the TensorRT version gate around getAliasedInputTensor is correct on both sides of 10.15 (checked by preprocessing it against several SDK header sets).
Uh oh!
There was an error while loading. Please reload this page.
| elif output_format == "executorch": | ||
| # retrace=True: torch.export truncates the engines' aliased KV | ||
| # outputs, so declare them as buffer mutations before lowering. | ||
| exp_program = _declare_aliased_kv_mutations_on_ep(exp_program) |
There was a problem hiding this comment.
Whether the mutation gets declared is decided twice, once by use_legacy_exporter inside _exporter.export and once by the outer retrace branch here, so two of the four combinations are wrong: retrace=True, use_legacy_exporter=True declares it twice and raises SpecViolationError: User output getitem_3 is not in the correct order, and retrace=False, use_legacy_exporter=False never declares it and prints no warning. Decide exposure once from the exporter that actually ran, and make _declare_aliased_kv_mutations_on_ep skip a buffer that already has a BUFFER_MUTATION spec.
There was a problem hiding this comment.
Turns out declaring on the exported_program branch created a third bad combination on top of your two that you found.
Fixed in the next push, using your suggestion and that covers all three combinations rather than special-casing any of them.
| continue | ||
| buf_node = input_nodes[ii] | ||
| buf_fqn = inputs_to_buffers.get(getattr(buf_node, "name", None)) | ||
| if buf_fqn is None or buf_fqn in already_exposed: |
There was a problem hiding this comment.
Two cases drop an aliased output here while the runtime still requires one delegate arg per engine output (TensorRTBackend.cpp:426): a second engine aliasing the same buffer is deduped, and a caller-supplied cache that is not a registered buffer gives buf_fqn is None and is skipped, both ending in Error::InvalidArgument at execute. The legacy path skips the same two at lines 765 to 793, so either emit one output arg per engine output, or record in the blob which outputs are not threaded so the runtime can adjust its expected arg count.
There was a problem hiding this comment.
I was able to confirm the second case, but I wasn't able to test the first.
I couldn't get a two-engine model that aliases one buffer through the retrace=True path. The export fails earlier, inside torch.export, on the hybrid TRT+eager graph. So I can't say whether that branch is reachable in practice. If you have a model in mind that hits it, that would help.
Regarding the fix, your second option looks workable for the non-buffer case specifically, since there's no ET buffer to write back to. I'd be wary of applying it generally though. An un-threaded aliased output also gets no write-back, and for a real buffer that turns an execute-time error into a silently stale cache. So scoping the blob-side elision to the non-buffer case and keeping one-arg-per-output elsewhere seems safer.
| } | ||
| void* dst = et_alias_out.nbytes() > 0 ? et_alias_out.mutable_data_ptr() : nullptr; | ||
| if (dst != nullptr && dst != bind_ptr) { | ||
| aliased_reflects.emplace_back(dst, bind_ptr, et_alias_out.nbytes()); |
There was a problem hiding this comment.
dst == bind_ptr cannot happen: the delegate input and output are live at the same time, so the memory planner never places them at the same address, and a mutable buffer is not in the planned arena at all. That makes the zero-copy path described in the comment and in the PR description dead, and every decode step pays a full cache-size device copy here plus ExecuTorch's write-back copy_ on a buffer the engine already updated in place, so it is worth stating whether that cost is inherent to expressing this as BUFFER_MUTATION.
There was a problem hiding this comment.
Confirmed dead. I instrumented the branch on a 30-layer export: 120 aliased outputs across prefill and decode, dst == bind_ptr false every time. The comment should go.
One correction: in our .pte the mutable buffers are planned values (mem_id 1, no mutable data segment). The reason dst != bind_ptr is the one you give, plus a second one: the delegate never receives the buffer at all. PropagateDevicePass wraps every delegate input in _h2d_copy, so the engine binds a per-call staging copy, and the reflect plus ExecuTorch's write-back exist to carry the update back to the buffer.
On whether the cost is inherent to BUFFER_MUTATION: it isn't. Pointing the mutation at the buffer placeholder does make ExecuTorch emit no copy_, but that alone breaks persistence, because the staging copy is re-made from the buffer at the top of every call, so the engine's in-place write is overwritten before it is read. Removing the staging too fixes it: the buffer lands in the device arena, the delegate binds it directly, and every full-cache copy goes away. On a single-layer KV model that is 16 -> 8 values and 10 -> 4 instructions, with the persistence check matching eager exactly (0.235384) where the elision-only build reported 0.
That is prototype-grade: a toy model, and the un-staging rule still needs a guard that every consumer of the buffer is device-capable.
The natural home looks like PropagateDevicePass itself, so the copy is never inserted for a delegate-mutated buffer in the first place, though that is more your call than mine. We could also carry it as a Torch-TensorRT pass that undoes the staging for our delegate only, if that sequences better, but it would have to hook in between PropagateDevicePass and memory planning, which is more fragile than doing it properly upstream.
Worth pursuing separately. For this PR I just corrected the comments. I kept the dst != bind_ptr check itself as a self-copy guard rather than deleting it, since it costs a pointer compare and is what stops a self-copy if planning ever changes.
There was a problem hiding this comment.
Fixed, verified on the current head (c87d3bbd8). The dst == bind_ptr comparison is gone; bind_ptr now appears only on the input-binding side, so the dead zero-copy branch and the comment claiming it no longer exist.
Please resolve this thread; I do not have write access to do it myself.
There was a problem hiding this comment.
Correcting my previous reply, which was wrong. I grepped for bind_ptr with a truncated output and only saw the input-binding uses, so I said the comparison was gone. It is not:
void* dst = et_alias_out.nbytes() > 0 ? et_alias_out.mutable_data_ptr() : nullptr;
// dst != bind_ptr guards against issuing a self-copy. The memory planner does// not currently place the delegate's output slot on the aliased input -- the// two are live at the same time -- so this holds for every aliased output.if (dst != nullptr && dst != bind_ptr) {What has changed is the comment, which now states the same thing I raised: the planner does not place them together, so the branch is always taken. So the misleading claim of a zero-copy fast path is gone, which was half my point.
The other half stands and is not fixed: every decode step still pays a full cache-size device copy here, plus ExecuTorch's write-back copy_. Please leave this thread open for that.
Conarnar
commented
Aug 14, 2026
Got it. The setup for the reference runner will be mirrored for kv_cache_decode_check. |
2e0c799 to
6b63746Compare28a3ec8 to
e8bd8e9Comparee8bd8e9 to
c87d3bbCompareAdds end-to-end caller-owned KV-cache support to the ExecuTorch TensorRT
delegate: the KV buffers are owned by the caller above the delegate and threaded
in as mutable-buffer delegate args, instead of being self-allocated inside a
(stateless) TensorRT engine.
Runtime + serialization (delegate):
- serialize each engine's aliased (KV-cache / in-place) I/O into the delegate blob
(serialization.py, backend.py, TensorRTBlobHeader.{h,cpp});
- at runtime bind each aliased TRT output binding to its aliased input's
caller-provided pointer (in-place) and reflect the result into the delegate
output EValue -- a no-op when the memory planner already aliased the two
(TensorRTBackend.{h,cpp}).
Export/lowering (torch_tensorrt):
- expose each engine's aliased outputs as graph-level BUFFER_MUTATIONs so
ExecuTorch keeps the KV buffers as caller-owned mutable buffers: at transform
time for the legacy exporter (retrace=False), and via a post-export pass
(_declare_aliased_kv_mutations_on_ep) for torch.export (retrace=True), which
otherwise truncates the aliased outputs at the fx boundary;
- keep delegate-mutated buffers above the delegate in TensorRTPartitioner
(tag_constant_data would otherwise freeze them as constants).
The retrace=True pass runs for exported_program as well as executorch. The
truncation happens at the fx boundary for every output format, so declaring only
on the executorch path left an exported_program saved with the mutation absent
from its signature while the engine still updated the cache in place. It is
declared before _normalize_engine_constants_to_python, which rewrites the engine
constants the pass reads aliased_io from. retrace=False was already correct for
every format via create_trt_exp_program. aot_inductor stays undeclared and
warns: whether an aliased in-place mutation survives functionalization under
inductor is unverified.
Tests cover serialization round-trip, the exposure-flag dispatch across both
retrace modes, the buffer-mutation declaration, and the partitioner un-tagging.…mutations
_declare_aliased_kv_mutations_on_ep rebuilds the ExportedProgram to attach the
new output specs, but reconstructed only root/graph/signature/state_dict/
range_constraints/module_call_graph/constants. example_inputs and verifiers are
not recoverable from the graph and reset to their defaults when omitted, so the
pass was not a drop-in replacement for the program it rewrites.
That was invisible while the pass ran only on the executorch path, since to_edge
does not read either. Declaring mutations for exported_program as well makes it
reachable: torch.export.save then persists a program whose example inputs are
silently gone. AOTI refuses such a program outright ("exported_program.
example_inputs is required to be set in order for AOTInductor compilation"), so
this also has to be fixed before that format can ever declare mutations.
Carry both through. The stub programs in test_kv_cache_export.py now model them,
and the capturing test asserts they reach the constructor.
Reported by cehongwang in review.Whether an aliased KV output gets declared a BUFFER_MUTATION is decided in two places: the exporter (the legacy one declares at transform time, via create_trt_exp_program) and save()'s per-output-format branch. Nothing reconciles them, so a program that arrives already declared is declared a second time. The duplicate spec then fails the ExportedProgram verifier's output ordering check. Seed already_exposed from the incoming signature's BUFFER_MUTATION targets rather than an empty set, so the pass skips buffers that are already declared and returns the program untouched when nothing new remains. That covers every combination that reaches it, including exported_program with use_legacy_exporter=True, which the preceding commit made reachable. The added test drives the pass on an already-declared program with ExportedProgram monkeypatched to raise, so a regression fails on "rebuilt the program" rather than on some later verifier complaint. The no-op fixture grows an output_specs field, which a real ExportGraphSignature always has. Reported by shoumikhin in review.
Four comments describe a zero-copy path where the memory planner places the delegate's output slot on the aliased input, so the reflect is skipped. That never happens: the delegate input and its aliased output are live at the same time, so the planner cannot co-locate them. Instrumenting the branch over a 30-layer export confirms it -- dst == bind_ptr was false for all 120 aliased outputs across a prefill and a decode step. Two of them go further and call the skipped case the common fast path, which inverts what the code does: every aliased output reflects, and a model with aliased outputs therefore always syncs before returning. Say what actually happens in all four (TensorRTBackend.h, and the reflect list, the reflect loop and the must_sync rationale in TensorRTBackend.cpp). The dst != bind_ptr check itself stays -- it is unreachable today, but it is what stops a self-copy if planning ever changes -- and its comment now says so instead of advertising a fast path. Also two comment fixes found in the same pass: "These two" in _exporter.py referred forward to checks the reader had not reached yet, and a test comment said "previously-dropped" where it meant the output torch.export truncates. No behaviour change. Reported by shoumikhin in review.
The aliased-output branch discarded the resize_tensor result and skipped the resize altogether when the rank was out of range, while the sibling non-aliased branch treats both as fatal. et_alias_out.nbytes() is read on the next line and sizes both the reflect D2D copy and, through the delegate output EValue, ExecuTorch's write-back copy_. A dynamic aliased output that outgrows its planned size would therefore move the stale planned byte count in both, silently truncating the cache update rather than failing. Mirror the sibling branch: reject an out-of-range rank and propagate a resize_tensor error. Reported by shoumikhin in review.
…th too Whether an aliased KV output is declared a BUFFER_MUTATION depends on two independent switches. save() picks the exporter (retrace, plus an optional use_legacy_exporter override) and only the legacy exporter exposes the mutations, at transform time; save()'s per-format branch declares them for everything else. The retrace=False branch did neither, so retrace=False with use_legacy_exporter=False produced a program that silently omits an update the engine performs -- no declaration and no diagnostic. Run the declaration pass on the exported_program and executorch branches there as well. The preceding commit made the pass skip buffers that already carry a spec, so this is correct for either exporter: the legacy one keeps declaring at transform time and the pass returns its program untouched. aot_inductor stays undeclared on both paths, as before. The added test drives save() over both formats and both exporters and asserts the pass runs exactly once; against the previous commit all four parametrizations fail. Reported by shoumikhin in review.
The runtime binds delegate output i to output_binding_names[i]. That holds because the partition's outputs are getitem(engine_node, i) in index order, and nothing verifies it: inputs are checked in _reorder_input_names_for_executorch, outputs were not. arrange_graph_outputs moves buffer mutations ahead of user outputs and is a no-op here only while the mutated buffers stay above the delegate, so a regression there would swap the serialized names silently. Validate the correspondence in preprocess. A single-output engine returned unwrapped is accepted -- one binding has no order to get wrong -- and anything else must be one getitem per binding, in index order. _build_edge_program only ever emitted `output((engine_node,))`, including for its three-output-binding case, which is not a shape that can occur: a three-tuple cannot be consumed as one value. It now emits one getitem per output binding, so the fixtures model what the backend actually receives. Reported by shoumikhin in review.
…d_io aliased_io changes what a blob means. A parser that predates it binds each aliased output to its own allocation instead of the input it aliases, so it does not fail -- it returns wrong results. The magic is the only field that parser validates, so it is the only thing that can make the skew fail closed. Emit TR02 when metadata.aliased_io is non-empty and keep TR01 otherwise, rather than bumping unconditionally: a blob with no alias map means exactly what it meant before, so it stays loadable by an older runtime. Both magics are accepted on read, so new runtimes still load existing artifacts. Verified against a real older build rather than a simulated one: a TR02 blob loads and produces the expected KV-persistence result on a runtime built from this branch, and a runtime built before the change rejects the same blob with "failed to parse TensorRT blob". Reported by shoumikhin in review.
…r verify The caller-owned KV path had no CI coverage: kv_cache_decode_check ships in the release tarball and is defined in the packaged CMake project, but nothing built or ran it, so a regression in the aliased binding would only surface downstream. Export a decode .pte alongside the static-shape one and pass it to the verify script as an optional second argument. When present the script builds kv_cache_decode_check from the unpacked tarball, runs it, and requires the persistence assertion to pass; the same no-libtorch link check the example runner gets is applied to it. Without the argument the script behaves as before. Also assert the tarball ships kv_cache_decode_check.cpp, next to the existing entries, so the packaging contract is checked rather than assumed. Reported by shoumikhin in review.
…ering claim Four `continue`s in _declare_aliased_kv_mutations_on_ep left an aliased output undeclared without saying so, and the mismatch only surfaced later as a delegate arity error at execute. Log at each, at the level the case warrants: warn when the persisted alias map disagrees with the engine's bindings (unknown input name, or an index past the delegate args) and when the aliased input is not a registered buffer, since all three leave the engine with an output binding the delegate cannot satisfy; debug when the buffer already carries a spec, which is the expected idempotent skip. The non-aliased output path stays silent -- it is the common case, not a fault. _reorder_input_names_for_executorch's docstring also justified skipping the output reordering by claiming a TensorRT partition has no mutation outputs. That has not been true since aliased I/O landed. The order does survive lowering, but for a different reason: _keep_mutated_buffers_above_delegate keeps mutated buffers out of the delegate, so ExecuTorch records the mutation as a USER_OUTPUT and arrange_graph_outputs computes the identity permutation. Say that, and note the guarantee is conditional -- _validate_output_binding_order is what enforces it. Reported by cehongwang in review.
c87d3bb to
d19c12dCompare- test_executorch_blob_header.cpp: fix `.output_name`/`.input_name` -> `.output`/`.input` (the struct fields; the test did not compile). - TensorRTBlobHeader.cpp: start the `"aliased_io"` search past the io_bindings array (`find(..., pos)`), so a model input literally named `aliased_io` no longer matches first and silently yields 0 aliased bindings. Also correct the inaccurate `"kind"`-default comment (the Python serializer always writes `kind`; older blobs have no aliased_io array at all). - _exporter.py `_declare_aliased_kv_mutations_on_ep`: guard the `torch_tensorrt.executorch.backend` import (which pulls `executorch.exir` at module scope) with try/except so a plain, non-executorch `save()` -- e.g. `output_format="exported_program"` for a model with no aliased KV outputs -- does not hard-require the optional `[executorch]` extra. Also fix the docstring: the pass runs on both retrace=True and retrace=False paths. - _compile.py: run `_declare_aliased_kv_mutations_on_ep` in the ExportedProgram save branch's `exported_program` sub-branch too, mirroring the fx branch (previously only the fx branch declared, so `save(ep, ...)` dropped the KV mutation declaration). - kv_cache_decode_check.cpp: cudaMemset the CUDA KV arenas to zero after alloc (ExecuTorch drops a mutated buffer's initial value, so the caches were never zeroed and two runs differed on uninitialized memory -> false PASS on this CI gate); add a NaN guard so NaN logits fail loudly instead of being reported as "identical"; update the README to match.
d19c12d to
e3482aaCompareThe aliased-I/O warning for `output_format="aot_inductor"` only fired on the `retrace=True` fx path. The two other `aot_inductor` entry points -- an input that is already an `ExportedProgram`, and the `retrace=False` fx path -- saved an aliased model with no signal that the format does not declare the in-place mutation. Add the same warning to both. The `ExportedProgram` branch has no `named_modules()`, so it iterates `module.graph_module.named_modules()`; the fx branch uses the in-scope GraphModule directly, matching the existing `retrace=True` check.
shoumikhin
commented
Aug 21, 2026
Cleaning up after myself: I had left a lot of long comments here, several already fixed and a couple simply wrong, so I deleted them and replaced them with this note. I left the inline threads where you replied, since your answers there are worth keeping. Nothing on this PR blocks merge from my side. Everything I raised is either fixed on the current head, which I re-checked, or parked with a reason I agree with. The two open ones are the arity mismatch, where half the case is still unproven and neither of us could build a two-engine repro, and the double-declare, which is really a Two corrections so nobody chases them:
The remaining review comments are on |
Uh oh!
There was an error while loading. Please reload this page.
Description
Adds end-to-end caller-owned KV-cache support to the ExecuTorch TensorRT delegate. The KV buffers are owned by the caller above the delegate and threaded through as mutable-buffer delegate args (both the input and the engine's aliased output), so a TensorRT engine updates them in place and the cache persists across decode steps — matching the contract the non-ExecuTorch TensorRT runtime already exposes.
Runtime + serialization (delegate)
serialization.py,backend.py,TensorRTBlobHeader.{h,cpp}).TensorRTBackend.{h,cpp}).kindmust be one we understand (kv_cache_update/user);kv_cache_updateentries are cross-checked against the engine's owngetAliasedInputTensor(TensorRT is the source of truth), anduserentries are shape-checked before two tensors are bound to the same storage. A caller-owned aliased input must be device-resident — otherwise its in-place update would be staged through host scratch and silently lost, so it's rejected loudly.execute()returns, since ExecuTorch's buffer-mutationcopy_reads the delegate output EValue afterward; the zero-copy caller-owned KV fast path records no reflect and is untouched.Export / lowering (torch_tensorrt)
BUFFER_MUTATIONs so ExecuTorch keeps the KV buffers as caller-owned mutable buffers instead of freezing them: at transform time for the legacy exporter (retrace=False), and via a post-export pass (_declare_aliased_kv_mutations_on_ep) fortorch.export(retrace=True), which otherwise drops the aliased outputs at the fx boundary. Theretrace=Truepass runs for bothexecutorchandexported_program;aot_inductoris left undeclared (and warns), since whether an aliased in-place mutation survives functionalization under inductor is unverified.TensorRTPartitioner(tag_constant_datawould otherwise freeze them as constants).Dependency
Depends on #4446 (the legacy
retrace=Falsesubmodule-inlining fix the composable ExecuTorch export path relies on), which has landed — this PR is now based onmain.Follow-up tests
Now that #4440 (per-method
TensorRTPartitioner→ separate delegate instances) and #4454 (shared caller CUDA stream, for ordering the dependent GPU work between two delegates) have both landed, a cross-delegate prefill/decode acceptance test — decode consuming the KV cache that prefill wrote through a separate per-method delegate — is unblocked and will be added as a follow-up. That configuration is what exercises cross-delegate cache sharing, which single-delegate tests cannot cover.Testing
aliased_ioserialization round-trip; blob-header parse (present / empty / missing-key, and a model input literally namedaliased_ionot mistaken for the array key); exposure-flag dispatch across both retrace modes; theBUFFER_MUTATIONdeclaration; partitioner keeps only mutation-target buffers above the delegate.examples/executorch_reference_runner/kv_cache_decode_check(exported byexport_kv_cache_decode.py) drives multi-step decode against the delegate and asserts the KV cache persists in place across steps, starting from an explicitly-zeroed cache (ExecuTorch drops a mutated buffer's initial value) with a NaN guard so the check can't pass vacuously.