Update Tensor* users for Graph::initializers_'s new unique_ptr storage - #9
Conversation
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.
|
CI status: the "Build whls" matrix is red across most platforms, but investigating a couple of them shows this is pre-existing breakage in this repo's own (long-diverged)
Verified via onnxsim's own full build+test suite instead (see PR description) — this repo's standalone CI wasn't green before this PR either (every other open PR here — #1 through #8 — is unmerged), so I'm not attempting to fix unrelated, pre-existing platform/test breakage as part of this small, bounded change. Generated by Claude Code |
Pulls in onnxsim/onnx#1 (Graph::initializers_: vector<Tensor> -> vector<unique_ptr<Tensor>>, so a Tensor's address survives other initializers being inserted/erased around it in the same vector) and the corresponding onnxsim/optimizer#9 call-site updates. See bench/RESULTS_issue633_followup.md for why this was worth doing (issue #633's follow-up) and why it's a bounded correctness/robustness fix, not a performance change on its own. Updates onnxsim.cpp's own EliminateZeroRnnInitialState, the one onnxsim-side caller of Graph::initializers() left after the onnx/ onnx-optimizer changes: initializers()[i] is now a unique_ptr<Tensor>, so building the by-name Tensor* lookup map needs .get() instead of &. Verified with a full rebuild against these exact submodule commits: onnxsim's core pytest suite (test_simple.py, test_backend.py, test_model_checking.py, test_constant_fold_determinism.py, test_moved_optimizer_passes.py) -- 51 passed, 2 failed on the pre-existing unrelated missing onnxscript dependency, 0 regressions -- plus end-to-end simplify() on cait_xxs36_224 (1758->1558 nodes) and mixer_l16_224_in21k (733->582 nodes), both ok=True and node-count identical to pre-bump output.
|
Same correction as onnxsim/onnx#1: my first validation pass accidentally ran against an unmodified onnxsim build (submodule-pointer sequencing mistake). Re-ran for real against this PR's actual diff (plus the corresponding onnx#1 commit and one more onnxsim-side fix, onnxsim/onnxsim#644) — 51/53 pytest passing (2 pre-existing unrelated failures), Generated by Claude Code |
Update: upstream fix landed, no changes needed hereonnxsim/onnx#1 (which this PR's call-site changes depend on) had a real data-loss bug in its first version: This PR's own diff ( Re-verified via onnxsim's full rebuild against the corresponding fixed commit on this repo's own Generated by Claude Code |
2 participants
Summary
onnx'sGraph::initializers_changed fromvector<Tensor>tovector<unique_ptr<Tensor>>so aTensor'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).Base branch note: targets
claude/tensor-hashing-research-iepmh2(the commit onnxsim'smasteractually hasthird_party/onnx-optimizerpinned to) rather thanmain, matching this repo's existing pattern for onnxsim-specific patches (see e.g. #8). Retarget tomainif/when that catches up.Changes
eliminate_duplicate_initializer.h: iterates the newvector<unique_ptr<Tensor>>, dereferencing one extra level. No behavior change — theCSETensorHash/CSETensorEqual-based dedup logic and its within-one-callTensorContentDigestcache usage are unaffected; this container swap does not, by itself, extend that cache's lifetime across calls (see onnxsim'sbench/RESULTS_issue633_followup.mdfor why that's a separate, larger, unattempted change).pass_util.h'sFetchConstantTensor:getInitializer()now returnsconst Tensor*directly instead of an iterator, so this drops the now-redundant&*.third_party/onnxsubmodule pin to the corresponding commit on theonnxside (onnxsim/onnx@781592f, a cherry-pick of the sameonnx/onnx#1fix onto this repo's existingthird_party/onnxlineage, since that lineage had already diverged from the one onnxsim's ownthird_party/onnxsubmodule tracks — see that commit for details) so this repo's own standalone build/tests stay consistent with these call-site changes.Testing
Verified via onnxsim's full rebuild (onnxsim's own
third_party/onnxandthird_party/onnx-optimizersubmodules pointed at the equivalent commits on the lineage onnxsim actually uses) plus its core pytest suite and end-to-endsimplify()runs — see onnxsim/onnx#1's PR description for the full validation (51/53 passing, 2 pre-existing unrelated failures;cait_xxs36_224/mixer_l16_224_in21koutput unchanged). This repo's own standalone build against the bumped submodule (this PR's actual diff) is left for CI to confirm.Generated by Claude Code