eliminate_deadend/eliminate_common_subexpression: avoid uses()'s O(N) capture scan - #11
Open
take-cheeze wants to merge 32 commits into
Open
eliminate_deadend/eliminate_common_subexpression: avoid uses()'s O(N) capture scan#11take-cheeze wants to merge 32 commits into
take-cheeze wants to merge 32 commits into
Conversation
MaxPool ignores its padded elements, which is equivalent to padding with -inf. The pass previously folded a Pad with constant_value=0 (or an unspecified value, which defaults to 0) into MaxPool by moving the padding into the pool's `pads` attribute. This changes the result whenever a pooling window's real values are all negative: Pad(0)+MaxPool yields 0 while the fused MaxPool yields the (negative) window maximum. Make the required Pad constant value depend on the pool type: - AveragePool (with count_include_pad=1): 0 - MaxPool: -inf Also restructure the constant-value check so an unspecified Pad value correctly blocks fusion into MaxPool instead of being treated as a match. Update the MaxPool tests that encoded the old behavior to assert no fusion for value=0/default, and add companion tests confirming Pad(value=-inf) folds correctly. Fixes onnxsim/onnxsim#290 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013sB8xKJyd86p47c2vfXMVD Signed-off-by: take-cheeze <takechi101010@gmail.com>
Add tests verifying that onnxoptimizer.optimize preserves model-local functions. Unlike the approach in onnx#199, which relied on torch.onnx.export(export_modules_as_functions=...) (a deprecated legacy exporter feature), these tests build functions directly with onnx.helper.make_function and the onnx text parser, matching the existing test idioms and avoiding a torch/torchvision dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0162JuH5K5FP2hCFYJRrKC49 Signed-off-by: Takeshi Watanabe <takechi101010@gmail.com> Signed-off-by: take-cheeze <takechi101010@gmail.com>
Extend the fuse_bn_into_conv pass to fold BatchNormalization into a preceding ConvTranspose, and fix the channel-axis bug that crashed on real models. ConvTranspose weight is laid out as (in_channels, out_channels, kH, kW), transposed relative to Conv's (out_channels, in_channels, kH, kW). The BatchNormalization channel count matches the output channels, so the per-channel scale must be checked and broadcast against axis 1 for ConvTranspose (axis 0 for Conv). Using axis 0 unconditionally triggered the reported conv_W.sizes()[0] == C assertion failure whenever in_channels != out_channels. Mismatched shapes (e.g. grouped ConvTranspose) now skip the fusion instead of asserting. The added test uses distinct in/out channel counts so it actually exercises the corrected axis. Squashed from onnx#316. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: take-cheeze <takechi101010@gmail.com>
Introduce a thread-local switch, SetInitializersAsConstants / InitializersAsConstants, that controls whether the fusion/elimination passes treat graph initializers as constant tensors. The single choke points IsConstantTensor and FetchConstantTensor consult it, so when the switch is off every value-baking pass (fuse_bn_into_conv, nop-reshape on a constant shape, ...) leaves initializer-backed weights untouched while Constant nodes stay constant. Expose it through the Python binding and as an initializers_as_constants keyword on onnxoptimizer.optimize(), which sets the switch around the run and restores it afterwards. Add tests covering the fuse-bn case and that the switch is restored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MyzBiNk5LvTaVPgURqD5UH
Implements two passes from the proposed_passes design drafts: fuse_consecutive_mul (default Fuse pass): Folds Mul(Mul(X, C1), C2) -> Mul(X, C1*C2) when C1 and C2 are constants and the inner Mul feeds only the outer one. The combined scale is materialised with numpy-style broadcasting, so the rewrite is numerically equivalent (X*C1*C2 at every position). Covers e.g. PoolFormer LayerScale exported as a per-channel (C,1,1) scale times a scalar factor. fuse_matmul_add_bias_into_gemm_batched (PassType::Other, opt-in only): Rewrites batched MatMul(X[>=3D], W[K,N]) + b into Reshape(->[-1,K]) -> Gemm -> Reshape(->[...,N]), extending the 2-D-only fuse_matmul_add_bias_into_gemm to the rank>=3 activations used by transformer linear layers. Static leading dims emit a constant output shape; dynamic dims rebuild it via Shape/Slice/Concat. Registered as PassType::Other so it stays out of the default fuse set, since it is a graph-shape rewrite that is not guaranteed to be faster. Adds unit tests for both passes (fuse, no-fuse guards, per-channel and dynamic-shape cases) with onnxruntime numeric-equivalence checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V63N6PXYEgNEe1BWbKi6cU
Fold a per-output-channel (or scalar) constant Mul that follows a Conv into the convolution weights: for `Z = Conv(X, W[, B]) * S`, rewrite to `Conv(X, W*S[, B*S])`. The scale is applied to the constant weights via Mul nodes that the constant folder then materialises, so the standalone multiply disappears; the trailing bias Add is subsequently absorbed by fuse_add_bias_into_conv, collapsing a `Conv -> Mul -> Add` affine tail (e.g. a BatchNorm exported as Mul/Add) back into the Conv. Only per-channel scales aligned to the Conv output channel axis (or a scalar) are fused; other shapes are left untouched. Only Conv (not ConvTranspose, whose weight layout differs) is handled. Bias, when present, must be constant so it can be scaled. On FasterRCNN-10 this removes 53 `Conv -> Mul -> Add` chains (106 nodes) that were previously left unfused. Adds tests covering per-channel and scalar scales (with and without bias) and a non-per-channel negative case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Piyb2V1gQuMBKfYdZeqqZy
Two pattern fusions onnxslim ships that onnxsim lacked (onnxsim issue #543): - fuse_add_bias_into_conv now also fuses a trailing bias `Add` into a ConvTranspose. The bias semantics are identical to Conv (a 1D tensor of length = output channels), but a ConvTranspose weight is laid out (in_ch, out_ch/group, k...) so its axis 0 is the input-channel count, not the output-channel count M. M is taken from the output shape and the weight-axis-0 shortcut is skipped for ConvTranspose. - eliminate_nop_dropout now removes an inference-mode Dropout expressed in the opset-12+ input form (previously only the pre-12 `ratio` attribute was handled). A Dropout is the identity whenever training_mode is false, so it is dropped when training_mode is omitted/constant-false and ratio is omitted/constant-0; a runtime training_mode input or nonzero ratio keeps it, and a consumed mask output blocks removal. Adds unit tests for both, including the grouped/kept negative cases. (ConvTranspose+BN fusion was already added separately in this optimizer.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014taCE8hEjqFEj6eFNyDJsn
…input shape The pass required the inner Unsqueeze's input to have a known shape, but that rank is only needed to normalize *negative* axes. On dynamic-shape graphs this blocked a valid fusion -- e.g. detection models (FasterRCNN) feed NonMaxSuppression through Unsqueeze(Unsqueeze(scalar)) chains whose input has no static shape, leaving ~80 redundant Unsqueezes onnxslim removes. Drop the has_sizes() precondition and instead bail inside runTransform only when the input shape is unknown *and* an axis is negative (the one case that truly needs the rank). Adds tests for the fuse and the negative-axis bail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014taCE8hEjqFEj6eFNyDJsn
…erand Add is commutative and exporters differ on operand order -- HuggingFace linear layers emit Add(bias, MatMul(x, W)) with the MatMul as the *second* operand, so the pass (which only matched CheckKind(Add, 0, MatMul)) never fired on real transformer graphs like bart. Detect the MatMul on either side and take the bias from the other. Adds a bias-first test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014taCE8hEjqFEj6eFNyDJsn
Update third_party/onnx from 777531d (1.22.0-dev) to 512e5d4 (1.23.0-dev) on top of the issue-543 optimizer passes, so downstream consumers (onnxsim) can pick up the newer ONNX while keeping the additional fusion passes on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZCiadh88bi9K2dVaxCRhS
fuse_matmul_add_bias_into_gemm_batched rewrites a rank-3 MatMul+bias into Reshape(-> 2-D) / Gemm / Reshape(-> N-D) so runtimes can dispatch tuned GEMM kernels. Between two such linears the inverse reshapes bracket a chain of element-wise ops and were left in the graph, inflating node counts on transformers (e.g. deit_tiny 380 -> 404, mobilebert 1672 -> 1865) even though the MatMul/Add counts drop. This Nop pass cancels that scaffolding: it matches the flattening Reshape a Gemm consumes, walks the closed element-wise region feeding it, and when the region bottoms out at the inverse unflatten reshapes (and trailing-broadcast constants) bypasses them so the region runs on the 2-D Gemm output. Both Gemms are kept; only the inverse reshape pairs drop. Shapes on the rewired region outputs are wiped so shape inference recomputes the 2-D shapes (a leftover 3-D value_info would otherwise trip onnxruntime's strict Gemm rank check). Registered as PassType::Nop, so it joins the default fuse/eliminate set and honours --skip-optimization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMofSUmZoFKfn2WNP1owUx
onnxsim owns these graph rewrites now and registers them into the global pass
registry at runtime, so drop them here and track upstream onnxoptimizer:
* New onnxsim-only passes (removed entirely):
- fuse_mul_into_conv
- fuse_consecutive_mul
- fuse_matmul_add_bias_into_gemm_batched
- eliminate_reshape_around_elementwise
* Passes onnxsim patched in place -- reverted to upstream here; onnxsim keeps
a patched copy and overwrites the registry entry (RegisterOrReplace):
- fuse_bn_into_conv (ConvTranspose)
- fuse_add_bias_into_conv (ConvTranspose)
- eliminate_nop_dropout (no-op opset-12 Dropout)
- fuse_pad_into_pool (zero-padding MaxPool fix)
- fuse_consecutive_unsqueezes (non-negative axes without static shape)
optimizer_test.py is reverted to upstream, keeping only the three
initializers-as-constants tests (that feature -- pass_util/optimize.h/cpp2py/
__init__ -- stays in the fork; onnxsim toggles it via SetInitializersAsConstants).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NmFXLPCnro8mTqC8vvx9zz
Repoints the vendored onnx submodule to https://github.com/onnxsim/onnx.git (pinned at the onnxsim/ir-name-uniqueness branch tip), so the Graph::isNameUnique() O(1) fix lives directly in the vendored source instead of being applied as a build-time patch by the onnxsim superproject. Otherwise identical to the previously-pinned onnx/onnx@512e5d4 -- this is the same commit plus that one fix.
Picks up onnxsim/onnx@85d8daba, which avoids a per-node attributeNames() heap allocation in Graph's subgraph-attribute walk (shared by Value::replaceAllUsesWith() and Graph::isNameUnique()). See that commit for the measured impact.
Pass::DescendOnGraphAttributesAndCount() and Pass::DescendOnGraphAttributesUnconstrained() are called once per node visited by PredicateBasedPass::_runPassInternal() -- i.e. for every fuse/ eliminate pass, every round of the optimizer's fixed point -- just to check for the (usually zero) g/gs (If/Loop/Scan) attributes on that node. attributeNames() heap-allocates a fresh vector per call to do this, on what is the hottest loop in the whole optimizer. Switch both to Attributes<Derived>::forEachAttributeNameAndKind() (added in onnxsim/onnx@85d8daba, picked up via the third_party/onnx submodule pin), a non-allocating (name, kind) visitor over the same underlying storage. No behavior change. Measured via onnxsim on the same 900-node Conv+BatchNorm+Relu benchmark used throughout this series: total simplification time dropped from 3.24s to 2.35s, for a combined ~3.72x over the original 8.76s baseline. Signed-off-by: Claude <noreply@anthropic.com>
The forEachAttributeNameAndKind() optimization (previous commit) assumed onnx-optimizer always compiles against the vendored onnx submodule (this fork), but onnxsim's ONNXSIM_BUILTIN_ORT=ON build path (used by the Rust bindings and standalone/WASM builds) links onnx-optimizer against a separate, unpatched onnx pulled in by onnxruntime's own CMake dependency chain -- which doesn't have forEachAttributeNameAndKind(), so that build fails with 'struct onnx::Node has no member named forEachAttributeNameAndKind'. Unlike the ir.h-internal changes (isNameUnique(), replaceAllUsesWith()'s subgraph walk), which are safe under either onnx copy since the whole change lives in one header, this one crosses a file boundary (onnx- optimizer's own pass.cc calling a method only guaranteed to exist in this fork's ir.h) that isn't safe to assume. Revert to the portable attributeNames() + kindOf() form; the two ir.h fixes captured the larger share of the measured win anyway. Signed-off-by: Claude <noreply@anthropic.com>
onnxsim/onnx's onnxsim/ir-name-uniqueness branch was rebased from onnx/onnx's old pinned commit (512e5d4e, 2026-07-29) onto current upstream main (7061d0d4, 2026-08-15), replaying the two fixes on top -- same content, 55 commits closer to upstream.
Picks up onnxsim/onnx@a2ef58c, which tracks subgraph-bearing nodes incrementally so isNameUnique() no longer scans every node in the graph on each call.
Ports the C++-core part of PR #3 (merged to main as 432d0e6) onto this branch, which main and this branch's history diverged before that PR was opened (this branch moved onnxsim-specific passes out into onnxsim/onnxsim and repoints third_party/onnx at onnxsim's fork; main still has the passes onnxsim removed here, so a straight cherry-pick or merge produces spurious conflicts across unrelated files). The four touched files -- pass_manager.h, pass_manager.cc, optimize.h, optimize.cc -- are byte-identical between the two branches at the common ancestor, so this is a hand-applied, verified-identical copy of that PR's diff to just those files; the Python-binding/test changes in the original PR are not needed here since onnxsim/onnxsim calls this library's C++ API directly, not its Python bindings. GeneralPassManager::run and FixedPointPassManager::run already compute a CountBasedPassAnalysis per pass internally (to know when a fixed point is reached); this exposes those counts via an optional out-parameter on Optimizer::optimize() / Optimize() / OptimizeFixed(), std::map<std::string, unsigned int>* report = nullptr, fully backward compatible. Motivation: onnxsim/onnxsim's Simplify() fixed point alternates shape inference and OptimizeFixed() calls, and after each one currently re-serializes and hashes the entire model just to detect whether anything changed, because there was no cheaper signal available. With this report, onnxsim can check whether transform_counts sums to zero and skip that convergence check (and the next round) when OptimizeFixed() genuinely made no changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U5Loz53Fkc1kFrqssiVidd
It was PassAnalysisType::Empty, so PassManager::run() silently excluded it from the transform_counts report added in the previous commit -- even though it does make real, uncounted graph changes (erasing unused initializers). That makes sum(transform_counts) == 0 an unsafe "the graph did not change" signal for any pass list that includes this pass, which GetFuseAndEliminationPass() does (it has PassType::Nop). Switch it to CountBased and report the number of initializers actually erased, matching the sibling eliminate_duplicate_initializer pass's pattern.
Optimizer::optimize() round-trips its input model through ModelProto -> Graph -> ModelProto, copying every initializer's raw bytes at both the Import and Export boundary. onnxsim's OptAndShape fixed point calls this once per round of its inner loop, so for deep, repeated-block models this copy dominates simplify() time (see onnxsim/onnxsim#633). Add a non-const Optimizer::optimize(ModelProto&, ...) overload (and matching Optimize/OptimizeFixed free-function overloads) that move each initializer's raw bytes through the round trip instead of copying them, via the consuming ImportModelProto/ExportModelProto overloads added to onnxsim's onnx fork. Only safe when the caller is about to discard or overwrite its input model, which onnxsim's usage already does on every iteration (`model = OptimizeFixed(model, ...)`). Gated behind ONNX_IR_PB_CONVERTER_HAS_CONSUMING_OVERLOADS, defined only by onnxsim's onnx fork, so this compiles unchanged when linked against onnxruntime's own bundled, unpatched onnx copy instead. The existing const-ref overloads are untouched. cpp2py_export.cc's bindings are pinned to the const overload explicitly (via static_cast<const ModelProto&>) so their behavior can't change silently through ordinary overload resolution merely because their local `proto` happens to be a non-const variable.
Ports onnx#319 (Optimizer::optimize(Graph&), OptimizeGraph, OptimizeGraphFixed): a C++ caller that already holds a Graph can now run the configured passes directly on it, with no ModelProto <-> Graph round trip at all. Requested for onnxsim issue #633, which tracks the remaining round-trip cost in onnxsim's OptAndShape fixed point after the tensor-byte-copy fix in #634. Extends the upstream PR with a `report` out-param on both the new Graph-native member and free functions, matching the existing ModelProto-based optimize()/Optimize()/OptimizeFixed() -- onnxsim's OptAndShape loop depends on that per-pass transform-count report for its fast-path "did anything change" signal, so a Graph-native entry point without it wouldn't be usable there. The two ModelProto-based Optimizer::optimize() overloads now delegate to the new optimize(Graph&, report) internally instead of calling pass_manager->run() directly, so they share one code path with the new entry point (same refactor upstream's PR makes). Unlike the consuming (moving) ModelProto overloads added for #634, OptimizeGraph/OptimizeGraphFixed never touch ModelProto or the onnx fork's Import/Export overloads at all, so they are NOT gated behind ONNX_IR_PB_CONVERTER_HAS_CONSUMING_OVERLOADS -- they work identically whether this library is linked against onnxsim's onnx fork or onnxruntime's bundled, unpatched onnx copy.
…aw_data CSETensorHash and CSETensorEqual (used by eliminate_duplicate_initializer and eliminate_common_subexpression) went through ParseTensorData<T> for every tensor comparison, which for raw_data-backed tensors makes two full copies of the tensor's bytes (one to un-const the string, one to convert it into a typed std::vector) plus an element-by-element hash_combine loop. Since raw_data is always little-endian on disk regardless of host byte order, byte-identical raw_data always implies value-identical data on any host. Add a fast path that hashes/compares the raw bytes directly via Tensor::raw() (a zero-copy const std::string&) when both tensors are raw_data-backed, skipping ParseTensorData entirely. A tensor whose duplicate happens to be stored via typed fields instead of raw_data (rare in practice) simply won't be recognized as a duplicate through this path -- a missed optimization, never an incorrect merge. On a 582-node ONNX model with ~300MB of raw_data initializers, eliminate_duplicate_initializer accounted for ~98% of all optimizer pass time (re-hashing every initializer from scratch on each of ~53 fixed-point rounds); this fix cuts total simplify() time roughly in half on that model.
cse_util.h's CSETensorHash/CSETensorCompare (used by EliminateDuplicateInitializer and the Constant-node CSE pass) get a TensorContentDigest (BLAKE3, vendored as a submodule under third_party/ blake3, mirroring onnxsim's own vendoring of the same library): one cryptographic hash over dtype+shape+values, computed once per tensor and covering both the raw_data and typed-field (float_data/int64_data/...) cases uniformly. CSETensorCompare trusts digest equality as tensor equality by default (GetTrustTensorContentHash() == true) rather than re-deriving/re-comparing every element -- for a typed-field tensor this cuts a comparison from a fresh ParseTensorData<T> copy down to a 32-byte compare. A false-positive match is not a practical concern given BLAKE3's cryptographic collision resistance, but SetTrustTensorContentHash(false) reverts to the original, exact element-by-element comparison for anyone who wants that guarantee instead. Also fixes a pre-existing, unrelated build break in tests/test_simple.cc (onnx::optimization::PrepareOutput -> onnx::PrepareOutput, following an upstream onnx namespace move) that blocked test_simple from building at all -- needed to get this change's own new test running.
TensorContentDigest hashed typed-field FLOAT/DOUBLE/COMPLEX64/COMPLEX128 values verbatim by bit pattern, but the pre-digest CSETensorHash/ CSETensorCompare it replaced hashed and compared them via std::hash<T>/ operator==, which (per IEEE754 and the C++ standard's hash/equality consistency requirement) treat +0.0 and -0.0 as equal. Since CSETensorHash always buckets by this digest regardless of the GetTrustTensorContentHash() setting, two Constant/initializer tensors differing only in the sign of a zero stopped landing in the same hash bucket at all -- so eliminate_common_subexpression/ eliminate_duplicate_initializer silently deduplicated fewer of them than before, under both trust=true and trust=false. This was caught by comparing model-regression.yml results before and after this branch's changes: several transformer models (albert, bert, bart, electra, mvp, three xcit variants) ended up with more remaining nodes than the pre-change baseline. Bisecting against the pre-digest commit with a local A/B build (two Constant nodes holding +0.0 and -0.0 via typed float_data, run through eliminate_common_subexpression) confirmed: old code merged them, new code didn't, under either trust setting. Fix: canonicalize -0.0 to +0.0 before hashing FLOAT/DOUBLE/COMPLEX64/ COMPLEX128 typed-field values (CanonicalizeZero in tensor_content_hash.cc). The raw_data fast path is untouched -- it already hashed disk bytes verbatim before this feature existed, so it never treated +0.0/-0.0 as equal either way. Float16/BFloat16 need no change: their operator==/hash already compare by bit pattern (see data_type.h), so they were never affected. Co-Authored-By: Claude <noreply@anthropic.com>
…aw_data Two sources of pure waste, found while investigating why this CSE change measured slower than expected on the model-regression suite: 1. TensorContentDigest was being called for raw_data tensors too, even though its own header comment already explains that path never needed it (raw_data hash/compare were already a one-shot std::hash/memcmp before this feature existed). Most real-model tensors are raw_data, so CSETensorHash/CSETensorCompare were paying full BLAKE3 cost on the dominant path for zero benefit. Restored the raw_data fast path to the original cheap byte hash/compare; BLAKE3 digests are now computed only for the typed-field path they were actually designed for. 2. Even for typed-field tensors, the digest was recomputed from scratch on every call: CSETensorHash computes it once per hash-bucket lookup, then CSETensorCompare recomputes it again for both sides on every candidate in that bucket -- and both EliminateDuplicateInitializer and EliminateCommonSubexpression separately doubled that by using a count()/find()-then-[]/at() pattern, which hashes (and, on a bucket hit, re-compares) the same tensor twice per lookup. Added a digest cache (ClearTensorContentDigestCache) scoped to one pass invocation -- safe because neither pass mutates a retained tensor's bytes mid-call, only nodes/edges -- and collapsed both count-then-insert patterns to a single emplace(). Co-Authored-By: Claude <noreply@anthropic.com>
onnx's Graph::initializers_ changed from vector<Tensor> to vector<unique_ptr<Tensor>> so a Tensor's address survives other initializers being inserted/erased around it in the same vector (see onnxsim/onnx#1, part of onnxsim issue #633's follow-up investigation into onnxsim's remaining speed gap vs onnxslim). Updates this repo's two call sites accordingly: - eliminate_duplicate_initializer.h: iterates the new vector<unique_ptr<Tensor>>, dereferencing one extra level. No behavior change -- the CSETensorHash/CSETensorEqual-based dedup logic and its within-one-call TensorContentDigest cache usage are unaffected; this container swap does not, by itself, extend that cache's lifetime across calls (see onnxsim's bench/RESULTS_issue633_followup.md for why that's a separate, larger, unattempted change). - pass_util.h's FetchConstantTensor: getInitializer() now returns const Tensor* directly instead of an iterator, so this drops the now-redundant &*. Bumps the third_party/onnx submodule pin to the corresponding commit on the onnx side (onnxsim/onnx@781592f, a cherry-pick of the same onnx/onnx#1 fix onto this repo's existing third_party/onnx lineage, since that lineage had already diverged from the one onnxsim's own third_party/onnx submodule tracks -- see that commit for details) so this repo's own standalone build/tests stay consistent with these call-site changes. Verified via onnxsim's full rebuild (onnxsim's own third_party/onnx and third_party/onnx-optimizer submodules pointed at the equivalent commits on the lineage onnxsim actually uses) plus its core pytest suite and end-to-end simplify() runs -- see onnxsim/onnx#1's PR description for the full validation. This repo's own standalone build against the bumped submodule is left for CI to confirm.
Previously, eliminate_duplicate_initializer and eliminate_common_ subexpression each cleared onnxoptimizer's TensorContentDigest cache (tensor_content_hash.h) at their own entry, because it was keyed by Tensor* -- only safe within one pass call, since a freed tensor's address can be reused by an unrelated, differently-contented tensor shortly after (Graph::eraseInitializer, an attribute being replaced). That forced re-hashing every retained tensor's content on every one of these two passes' calls, even when nothing about that tensor had changed -- the dominant cost on deep repeated-block models whose OptAndShape fixed point (onnxsim) can take dozens of rounds to converge (onnxsim issue #633). Depends on onnxsim/onnx@debe14bb (Tensor::tensor_id()): a per-object id minted fresh on every construction and assignment, so it can never alias two different tensors' content the way a reused address could. Re-keying the cache on tensor_id() instead of &tensor removes that constraint structurally, so: - eliminate_duplicate_initializer/eliminate_common_subexpression no longer clear the cache themselves. - Optimizer::optimize(Graph&, ...) (and the OptimizeGraph/ OptimizeGraphFixed free functions wrapping it) gained a clear_tensor_digest_cache parameter, defaulting to true: it clears the cache at entry, bounding its memory to one optimize() call -- the same safety envelope every existing caller already had, just applied once per call instead of once per pass invocation within it (itself already a real, if smaller, win for everyone). - A caller that holds one Graph across several optimize() calls (e.g. onnxsim's OptAndShape, which now clears the cache itself once at the top of its own fixed point and passes clear_tensor_digest_cache=false to every OptimizeGraphFixed call inside it) can opt out to keep digests warm across all of them instead. Bumps third_party/onnx to the corresponding tensor_id() commit, cherry-picked onto this repo's own (diverged) third_party/onnx lineage as onnxsim/onnx@c3b180f3, matching the existing pattern for this repo's onnxsim-specific patches. Signed-off-by: Claude <noreply@anthropic.com>
…neck eliminate_duplicate_initializer/eliminate_common_subexpression's hash-map lookups had two hashing paths: TensorContentDigest (typed-field tensors, already cached across rounds since the previous commit) and CSETensorHash's raw_data branch (a std::hash<std::string> scan of the tensor's raw bytes, uncached). Profiling onnxsim issue #633's mixer_l16_224_in21k repro showed raw_data is overwhelmingly the exercised path on real exported models (15929 raw_data hash calls vs 1193 typed-field), and that hashing -- not comparison, which was already negligible at 0.13ms total -- accounted for 73% of the entire Optimize() phase: eliminate_duplicate_initializer alone was ~10.4s of a ~14.1s Optimize() call, ~10.3s of which was raw_data hashing. 98%+ of those hash calls were recomputing a value already seen earlier in the same run. Adds g_raw_hash_cache (cse_util.h), keyed by Tensor::tensor_id() for the same freed-pointer-reuse safety as TensorContentDigest's cache: a Tensor's content is never mutated in place by any onnx-optimizer pass (only replaced wholesale, which mints a fresh tensor_id() and simply misses this cache), so a hash computed for a tensor that survives unchanged across rounds stays valid. Threaded through Optimizer::optimize(Graph&, ...)'s existing clear_tensor_digest_cache parameter alongside ClearTensorContentDigestCache, so callers that already opt out of clearing one cache per round (e.g. onnxsim's OptAndShape) get both. Also lands the pass-phase profiling instrumentation used to find this (pass.h/pass.cc/pass_manager.cc's PassPhaseTiming/PassTotalTiming, cse_util.h's CSEHashCompareTiming) -- off by default (SetPassPhaseProfilingEnabled), zero overhead when disabled, gated behind onnxsim's ONNXSIM_PROFILE_PASS_PHASES env var. Kept as a permanent, documented diagnostic since it was instrumental in finding this and the existing TensorContentDigest caching was, on its own, ineffective (it only covered the rare typed-field path). Measured on mixer_l16_224_in21k_Opset17 (833MB, the flagship #633 repro), same host, before/after this commit: eliminate_duplicate_initializer 10446ms -> 648ms, Optimize() phase 14200ms -> 4343ms, end-to-end wall time 46.68s -> 37.5s (~20% faster), 93.23% cache hit rate (14850/15929 calls). Output node count and check_ok unchanged (582 nodes, ok=True). cait_xxs36 (where this pass isn't dominant) unaffected: 1758->1558 nodes, ok=True, ~23s, matching its pre-change baseline. Signed-off-by: Claude <noreply@anthropic.com>
…nals Follow-up profiling for onnxsim issue #633: after the raw_hash_cache fix, eliminate_common_subexpression and eliminate_deadend became the two heaviest passes in Optimize() (1399ms and 1310ms of ~4507ms total on mixer_l16_224_in21k_Opset17, vs eliminate_duplicate_initializer's now- reduced 661ms). Neither cost is explained by CSETensorHash/CSETensorCompare (589.5ms/0.06ms total, mostly attributed to eliminate_duplicate_initializer), so this adds instrumentation to find out what actually dominates each: - cse_util.h: CSEHashCompareTiming gains node_hash_*/node_equal_* fields. CSENodeHash::operator() and CSEEqual::operator() are now timed as a whole, plus their attributeNames()+std::sort sub-step is timed separately (attributeNames() returns std::vector<Symbol> by value -- an allocation -- on every call; this isolates whether that allocation is the actual cost). - pass.h/pass.cc: new CSEPassTiming (filter/lookup/replace phases of EliminateCommonSubexpressions's own loop) and DeadendPassTiming (hasUses()/destroyCurrent() phases of EliminateDead's loop), same on/off toggle as the existing pass-phase timers. - eliminate_common_subexpression.h / eliminate_deadend.h: wire up the above at their respective call sites. Off by default, zero std::chrono overhead when disabled.
… capture scan Both passes call node->hasUses() once per node in a full-graph loop. hasUses() calls Value::uses(), which -- on top of an O(1) same-graph use list -- always pays a full owningGraph()->forEachNode() scan (recursing into every nested subgraph) looking for a kCaptured placeholder that might reference this value from inside an If/Loop/Scan body. For a graph with no control-flow ops at all (the overwhelming majority of real models), that scan can never find anything, but both passes were still paying it on every node of every one of their O(nodes)-sized loops -- an accidental O(nodes^2) cost hiding behind what looks like a linear pass. Compute once per pass invocation (not once per node) whether the graph tree contains any kCaptured node at all (GraphMayHaveCapturedValues, new in pass_util.h); when it doesn't, use the new O(1) Node::hasUsesInCurrentGraph() (onnxsim/onnx#3) instead of hasUses(), falling back to the exact previous, correct behavior whenever a capture is possible. Profiling onnxmodelzoo/cait_xxs36_224_Opset18 (36 repeated transformer blocks, one of the two models flagged as still lagging onnxslim in onnxsim issue #651) showed eliminate_common_subexpression's node filter and eliminate_deadend's hasUses() loop as ~65% of the whole Optimize() phase's cost across the model's ~54 simplification rounds -- each doing a full graph scan per node, every round. Fixing this measured onnxsim's own simplify() end-to-end time roughly halved on that model (same output node count, no behavior change) and ~20% faster on onnxmodelzoo/swin_s_Opset18 (the other flagged model, and the largest model in onnxsim's whole regression set). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADw2idu5qWKonBuaBcJEP4
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on onnxsim/onnx#3 (adds the
Node::hasUsesInCurrentGraph()this depends on).Both
eliminate_deadendandeliminate_common_subexpressioncallnode->hasUses()once per node in a full-graph loop.hasUses()callsValue::uses(), which -- on top of an O(1) same-graph use list -- always pays a fullowningGraph()->forEachNode()scan (recursing into every nested subgraph) looking for akCapturedplaceholder that might reference this value from inside an If/Loop/Scan body. For a graph with no control-flow ops at all (the overwhelming majority of real models), that scan can never find anything, but both passes were still paying it on every node of every one of their O(nodes)-sized loops -- an accidental O(nodes²) cost hiding behind what looks like a linear pass.Compute once per pass invocation (not once per node) whether the graph tree contains any
kCapturednode at all (GraphMayHaveCapturedValues, new inpass_util.h); when it doesn't, use the new O(1)Node::hasUsesInCurrentGraph()instead ofhasUses(), falling back to the exact previous, correct behavior whenever a capture is possible.Measured impact
Profiling
onnxmodelzoo/cait_xxs36_224_Opset18(36 repeated transformer blocks, one of two models flagged in onnxsim/onnxsim#651 as still lagging onnxslim) showedeliminate_common_subexpression's node filter andeliminate_deadend'shasUses()loop as ~65% of the wholeOptimize()phase's cost across the model's ~54 simplification rounds -- each doing a full graph scan per node, every round.Via onnxsim's own
simplify(), end-to-end (same output node count both before/after -- no behavior change):cait_xxs36_224: roughly halved (~16.5s → ~7.8s locally)swin_s(the other flagged model, and the largest model in onnxsim's whole regression set): ~20% faster (~115s → ~87-93s locally)Test plan
test_simple.py,test_python_api.py,test_constant_fold_determinism.py,test_function_rewriter_vs_onnxscript.py,test_profiling.py): 75 passed (2 known pre-existing/environmental failures unrelated to this change, a missing optionalonnxscriptdependency)cait_xxs36_224/swin_send-to-end: identical output node counts to the pre-change baseline (1558 and 1034 respectively),ok=TrueGenerated by Claude Code