feat: add native CPU kernel for SequenceMap (opset 17) - #28813
feat: add native CPU kernel for SequenceMap (opset 17)#28813Rishi-Dave wants to merge 3 commits into
Conversation
Replace the broad ORT_USE_CPUINFO macro (with negated platform exclusions) with inline defined(CPUINFO_SUPPORTED) && defined(__linux__) guards at each point of use. Since __APPLE__ and __linux__ are mutually exclusive, the previous negation-based condition collapses to simply defined(__linux__). Drop the intermediate ORT_USE_CPUINFO macro in favour of direct guards.
Without a dedicated kernel, SequenceMap falls back to the ONNX context-dependent function body, which expands the op into a Loop over SequenceInsert calls. Each SequenceInsert copies the accumulator sequence, producing O(n^2) memory traffic for an n-element input. This adds a native CPU kernel that: - Derives from IControlFlowKernel and sets up the body subgraph via the standard FeedsFetchesManager flow used by Loop, If, and Scan. - Iterates the input sequence sequentially in O(n), forwarding the i-th element of each sequence-typed input and passing tensor-typed additional_inputs through unchanged. - Validates that sequence-typed additional_inputs share the input sequence length. - Assembles one TensorSeq per body output and appends fetched tensors per iteration without intermediate copies. Adds unit tests for the identity body, an add-scalar body with a tensor additional input, and a body that emits two outputs to cover the multi-output path. Fixes microsoft#23024
There was a problem hiding this comment.
Pull request overview
This PR adds a native CPU Execution Provider kernel for ONNX SequenceMap (opset 17) to avoid the quadratic Loop + SequenceInsert function-body fallback, executing the body subgraph directly per sequence element via the control-flow kernel infrastructure.
Changes:
- Introduces
SequenceMapas a CPU control-flow kernel usingFeedsFetchesManager+utils::ExecuteSubgraph. - Registers the new kernel in the CPU EP opset-17 registry.
- Adds unit tests that construct minimal
bodysubgraphs (identity, add-with-extra-input, two outputs) and validate results. - Also changes cpuinfo usage gating in
PosixEnvto Linux-only.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/core/providers/cpu/sequence/sequence_ops.h | Declares the new SequenceMap control-flow kernel and its FeedsFetchesManager member. |
| onnxruntime/core/providers/cpu/sequence/sequence_ops.cc | Implements SetupSubgraphExecutionInfo/Compute and registers the opset-17 CPU kernel. |
| onnxruntime/core/providers/cpu/cpu_execution_provider.cc | Registers SequenceMap in the CPU EP kernel registry. |
| onnxruntime/test/providers/cpu/sequence/sequence_ops_test.cc | Adds new SequenceMap unit tests with constructed body subgraphs. |
| onnxruntime/core/platform/posix/env.cc | Narrows cpuinfo integration to Linux-only in PosixEnv. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto& subgraph_map = subgraph_session_state.GetOrtValueNameIdxMap(); | ||
| std::unique_ptr<FeedsFetchesManager> ffm; | ||
| ORT_RETURN_IF_ERROR(FeedsFetchesManager::Create(feed_names, fetch_names, subgraph_map, ffm)); | ||
| ORT_RETURN_IF_ERROR(utils::InitializeFeedFetchCopyInfo(subgraph_session_state, *ffm)); |
| std::vector<std::string> outer_feed_names; | ||
| outer_feed_names.reserve(node_inputs.size()); | ||
| for (const auto* input_def : node_inputs) { | ||
| outer_feed_names.push_back(input_def->Name()); | ||
| } |
| std::vector<OrtValue> feeds; | ||
| feeds.reserve(static_cast<size_t>(num_outer_inputs)); | ||
|
|
||
| // Build feeds: sequence inputs -> element i; tensor inputs -> pass-through OrtValue. | ||
| for (int k = 0; k < num_outer_inputs; ++k) { | ||
| const auto* seq_k = (k == 0) ? input_seq : ctx->Input<TensorSeq>(k); | ||
| if (seq_k != nullptr) { | ||
| feeds.push_back(seq_k->GetAt(i)); | ||
| } else { | ||
| // Tensor input: shallow-copy the OrtValue (shared_ptr, safe) from the kernel context. | ||
| const auto* input_val = ctx_internal->GetInputMLValue(k); | ||
| ORT_ENFORCE(input_val != nullptr, "SequenceMap: input ", k, " is neither a sequence nor a tensor."); | ||
| feeds.push_back(*input_val); | ||
| } | ||
| } |
| for (int j = 0; j < num_outputs; ++j) { | ||
| output_seqs[j] = ctx->Output<TensorSeq>(j); | ||
| ORT_ENFORCE(output_seqs[j] != nullptr, "SequenceMap: failed to get output TensorSeq slot ", j); | ||
| output_seqs[j]->Reserve(seq_len); | ||
| } |
| if (i == 0) { | ||
| output_seqs[j]->SetType(fetches[j].Get<Tensor>().DataType()); | ||
| } |
| TypeProto float_tensor; | ||
| float_tensor.mutable_tensor_type()->set_elem_type(TensorProto_DataType_FLOAT); | ||
| float_tensor.mutable_tensor_type()->mutable_shape()->add_dim(); | ||
|
|
||
| auto& x_arg = graph.GetOrCreateNodeArg("x", &float_tensor); | ||
| auto& scalar_arg = graph.GetOrCreateNodeArg("scalar_in", &float_tensor); | ||
| auto& out_arg = graph.GetOrCreateNodeArg("add_out", &float_tensor); |
| // additional_inputs is a tensor (passed through to every iteration) | ||
| test.AddInput<float>("additional_inputs", {3}, {100.0f, 100.0f, 100.0f}); |
| // We can not use CPUINFO if it is not supported and we do not want to use | ||
| // it on certain platforms because of the binary size increase. | ||
| // We could use it to find out the number of physical cores for certain supported platforms | ||
| #if defined(CPUINFO_SUPPORTED) && !defined(__APPLE__) && !defined(__ANDROID__) && !defined(__wasm__) && !defined(_AIX) | ||
| #if defined(CPUINFO_SUPPORTED) && defined(__linux__) | ||
| #include <cpuinfo.h> | ||
| #define ORT_USE_CPUINFO | ||
| #endif |
There was a problem hiding this comment.
Changes here are out of scope with the PR and largely unjustified. The readability is also a concern.
| for (size_t i = 0; i < seq_len; ++i) { | ||
| std::vector<OrtValue> feeds; | ||
| feeds.reserve(static_cast<size_t>(num_outer_inputs)); |
- Include implicit inputs in subgraph feed setup and Compute - Initialize output TensorSeq element type before iteration loop so empty input sequences produce correctly-typed outputs - Remove redundant per-iteration SetType - Hoist feeds/fetches allocations outside the iteration loop - Fix scalar broadcasting test: use rank-0 TypeProto and scalar value
|
Thanks for the review. Addressed in 62a1bea:
On the |
tianleiwu
left a comment
There was a problem hiding this comment.
Summary
The core change — a native CPU SequenceMap kernel that runs the body subgraph once per sequence element instead of expanding into the O(n^2) Loop + SequenceInsert function-body fallback — is well implemented and faithfully mirrors the established control-flow kernel pattern (Scan/Loop/If):
SetupSubgraphExecutionInfobuilds theFeedsFetchesManagerfrom subgraph input names + implicit defs, discovers feed devices from the outer node inputs + implicit defs, and finalizes copy info exactly likescan_utils.cc::CreateFeedsFetchesManager.Computehoistsfeeds/fetchesoutside the iteration loop, forwards implicit inputs inImplicitInputDefs()order, sets each outputTensorSeqelement type before the loop (so an empty input sequence still produces correctly-typed outputs), and validates additional-sequence length parity.
The feedback from the prior review round appears addressed in this head: implicit-input handling in both setup and compute, empty-sequence output typing, the rank-0 scalar broadcast test, and per-iteration heap churn.
Findings
Out of scope (high): core/platform/posix/env.cc cpuinfo gating change. This PR also narrows cpuinfo gating from CPUINFO_SUPPORTED && !__APPLE__ && !__ANDROID__ && !__wasm__ && !_AIX to CPUINFO_SUPPORTED && __linux__ and drops the ORT_USE_CPUINFO macro — unrelated to SequenceMap, and already flagged in an existing thread. Beyond scope, it is a behavioral narrowing: any non-Linux POSIX platform that previously satisfied the old guard (e.g. the BSDs / Solaris when CPUINFO_SUPPORTED is defined) now silently loses cpuinfo-based physical-core detection and falls back to DefaultNumCores(). If the intent is to fix a non-Linux build break (the affinity block uses Linux-specific logic), please document that rationale and ship it as a separate PR. Recommend reverting it from this PR.
Nitpick: prefer Status over ORT_ENFORCE for body-result mismatches. In Compute, the body-output-count and per-output IsTensor() checks use ORT_ENFORCE (throws). These are effectively model/body validation errors; returning ORT_MAKE_STATUS(..., INVALID_ARGUMENT, ...) — as already done for the length-parity check — would surface them more gracefully and consistently.
Positives
- Correct reuse of
OpKernelContextInternal::SubgraphSessionState("body"),GetImplicitInputs(), andGetInputMLValue(). - Moving output typing ahead of the loop fixes the empty-sequence edge case.
- Tests cover identity pass-through, rank-0 scalar broadcast via an extra input, and a two-output body graph.
tianleiwu
left a comment
There was a problem hiding this comment.
Thanks for tackling the O(n^2) SequenceMap fallback — the control-flow structure (FeedsFetchesManager + ExecuteSubgraph, implicit-input forwarding, feeds/fetches hoisting) mirrors Loop/Scan/If correctly. However, I applied this branch locally and built/ran onnxruntime_provider_test --gtest_filter='SequenceOpsTest.SequenceMap*', and there are three blocking issues that currently make the Test Plan non-reproducible:
- Build break:
ctx->GetTerminateFlag()does not compile —GetTerminateFlag()lives onOpKernelContextInternal, not the baseOpKernelContext. Usectx_internal->GetTerminateFlag(). - Runtime crash on all three tests: setting the output element type from
ctx->OutputType(j)->AsSequenceTensorType()->GetElementType()throwsGetElementType is not implemented, because theAllTensorAndSequenceTensorTypes()constraint makesOutputType(j)the type-erasedSequenceTensorTypeBase. The element type must come from the runtime body output tensor. - Tensor pass-through is unreachable:
ctx->Input<TensorSeq>(k)throws (Missing Input) on a plainTensorinput rather than returningnullptr, soSequenceMap_AddScalarfails. Distinguish viaGetInputMLValue(k)->IsTensorSequence().
After applying minimal fixes for all three, SequenceMap_Identity and SequenceMap_TwoOutputs pass and SequenceMap_AddScalar passes once #3 is addressed — so the core approach is sound, it just needs these corrected and the tests actually run before merge. Inline details below. (The out-of-scope env.cc cpuinfo change has already been raised and isn't repeated here.)
| ORT_RETURN_IF_ERROR(utils::ExecuteSubgraph(*session_state, *feeds_fetches_manager_, | ||
| feeds, fetches, {}, | ||
| ExecutionMode::ORT_SEQUENTIAL, | ||
| ctx->GetTerminateFlag(), |
There was a problem hiding this comment.
Build break. GetTerminateFlag() is declared only on OpKernelContextInternal (op_kernel_context_internal.h), not the base OpKernelContext. As written this fails to compile: error: 'class onnxruntime::OpKernelContext' has no member named 'GetTerminateFlag'. ctx_internal is already available a few lines up — use ctx_internal->GetTerminateFlag() (the same way if.cc/loop.cc/scan_utils.cc call it on the internal context).
| ORT_ENFORCE(output_seqs[j] != nullptr, "SequenceMap: failed to get output TensorSeq slot ", j); | ||
| const auto* out_type = ctx->OutputType(j); | ||
| ORT_ENFORCE(out_type != nullptr, "SequenceMap: could not determine type for output ", j); | ||
| output_seqs[j]->SetType(out_type->AsSequenceTensorType()->GetElementType()); |
There was a problem hiding this comment.
Runtime crash on every test. Because the V type constraint is AllTensorAndSequenceTensorTypes(), ctx->OutputType(j) is the type-erased SequenceTensorTypeBase, whose GetElementType() is ORT_NOT_IMPLEMENTED. After fixing the GetTerminateFlag build break, all three tests fail here with Status Message: GetElementType is not implemented.
The output element type has to come from the actual runtime body output, e.g. set output_seqs[j]->SetType(fetches[j].Get<Tensor>().DataType()) on the first iteration. Note this means the empty-sequence (seq_len==0) typing that the earlier "change D" tried to solve via OutputType cannot be satisfied this way — there are no body outputs to infer from, so that edge case needs a different approach (or to be documented as untyped).
|
|
||
| // Build feeds: sequence inputs -> element i; tensor inputs -> pass-through OrtValue. | ||
| for (int k = 0; k < num_outer_inputs; ++k) { | ||
| const auto* seq_k = (k == 0) ? input_seq : ctx->Input<TensorSeq>(k); |
There was a problem hiding this comment.
Tensor pass-through branch is unreachable; throws instead. OpKernelContext::Input<T>() returns nullptr only when an input is absent. For a present input of the wrong type it calls OrtValue::Get<TensorSeq>(), which ORT_ENFORCEs the type and throws, re-thrown as Missing Input. So for a plain Tensor additional_inputs, this line throws rather than taking the else (pass-through) branch. Verified: after fixing the two issues above, SequenceMap_AddScalar fails with Input(int) const [with T = onnxruntime::TensorSeq] Missing Input: additional_inputs.
Detect the input kind via the OrtValue type instead:
const OrtValue* val = ctx_internal->GetInputMLValue(k);
const TensorSeq* seq_k = (val && val->IsTensorSequence()) ? &val->Get<TensorSeq>() : nullptr;The same fix is needed in the length-validation loop at line 692, which has the identical ctx->Input<TensorSeq>(k) pattern.
Summary
SequenceMap(opset 17), eliminating the ONNX function-body fallback that expands into aLoopoverSequenceInsertand produces O(n^2) memory traffic.Loop/If/Scan: derives fromIControlFlowKernel, prepares feeds/fetches viaFeedsFetchesManager, and invokes the body subgraph throughutils::ExecuteSubgraph.Motivation
Fixes #23024. The ONNX spec defines
SequenceMapvia a context-dependent function body that decomposes the op into aLoopwhose accumulator is grown bySequenceInserton every iteration. EachSequenceInsertcopies the accumulated sequence, so processing an n-element input requires O(n^2) memory traffic. Workloads that map per-element transforms over long sequences hit this quadratic behaviour and are forced to avoid the operator entirely.A native kernel iterates the input in O(n), forwards the i-th element of each sequence-typed input plus passthrough tensor inputs to the body, and appends each body output to the appropriate output
TensorSeqwithout per-iteration copies.Changes
onnxruntime/core/providers/cpu/sequence/sequence_ops.h: declaresSequenceMapas anIControlFlowKernelwith aFeedsFetchesManagermember.onnxruntime/core/providers/cpu/sequence/sequence_ops.cc: implementsSetupSubgraphExecutionInfoandCompute, registers the kernel for opset 17, validates length parity for sequence-typedadditional_inputs, and assembles per-outputTensorSeqresults.onnxruntime/core/providers/cpu/cpu_execution_provider.cc: adds the forward declaration andBuildKernelCreateInfoentry for the new kernel alongside the other opset-17 sequence ops.onnxruntime/test/providers/cpu/sequence/sequence_ops_test.cc: addsSequenceMap_Identity,SequenceMap_AddScalar, andSequenceMap_TwoOutputscovering single-input identity, sequence + tensor broadcast, and dual-output body graphs.Test Plan
onnxruntime_test_all --gtest_filter='SequenceOpsTest.SequenceMap*'— exercises the three new tests.onnxruntime_test_all --gtest_filter='SequenceOpsTest.*'— confirms no regression in sibling sequence ops.sequence_map_identity_*,sequence_map_add_*, andsequence_map_extract_shapescontinue to be excluded for TensorRT EP only; the CPU EP now executes them via the native kernel rather than the function-body fallback.Issue Resolution
Fixes #23024.