Skip to content

Fix dead dedup guard in GraphTransformerManager::Register (audit F10) - #29556

Merged
GopalakrishnanN merged 3 commits into
mainfrom
GopalakrishnanN/graph-transformer-register-dedup
Jul 30, 2026
Merged

Fix dead dedup guard in GraphTransformerManager::Register (audit F10)#29556
GopalakrishnanN merged 3 commits into
mainfrom
GopalakrishnanN/graph-transformer-register-dedup

Conversation

@GopalakrishnanN

@GopalakrishnanN GopalakrishnanN commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Description

GraphTransformerManager::Register guarded against duplicate registration with a std::find over level_to_transformer_map_[level] (a container of std::unique_ptr<GraphTransformer>) looking for the transformer unique_ptr about to be inserted. Since that unique_ptr is not yet in the container and owns a distinct pointer, the comparison can never match — the guard is dead code.

As a result, a transformer registered with a duplicate name at the same level slips through: it is appended a second time to level_to_transformer_map_[level] and applied twice per optimization pass, and the intended "already registered" error is never returned.

Fix

Replace the dead identity check with a per-level name check backed by an O(1) hash lookup: each transformer's name is recorded in the existing transformers_info_ map keyed by (level, name), so a duplicate at the same level is rejected in constant time instead of scanning the transformers already registered at that level (avoiding the quadratic registration cost). This preserves the original per-level intent — the same transformer name at different levels remains valid (e.g. LayerNormFusion is registered at both Level1 and Level2 by GenerateTransformers), which a global-name check would have wrongly rejected.

Tests

Adds GraphTransformationTests.RegisterDuplicateTransformerNameFailsPerLevel:

  • same name, same level → FAIL
  • same name, different level → OK

Verified locally: the test fails on the pre-fix code (duplicate_status.IsOK() was true, i.e. the duplicate was wrongly accepted) and passes with the fix. The full GraphTransformationTests suite remains green after the O(1) revision — re-verified on a RelWithDebInfo build (350 passed, 15 CUDA/WebGpu skipped, 0 failed), including the layernorm tests that register the same fusion at Level1 and Level2.

Motivation and Context

Fixes the design-audit F10 finding — a non-functional safeguard that silently allowed duplicate transformer registration and double-application.

The std::find compared the incoming unique_ptr against the stored ones, which can never match, so duplicate transformer names were silently accepted and applied twice per level. Use a per-level name check (same name at different levels stays valid). Adds a regression test.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a non-functional duplicate-registration guard in GraphTransformerManager::Register so that duplicate transformer names at the same optimization level are correctly rejected, preventing accidental double-application of the same transformer within a level.

Changes:

  • Replace the dead std::find check (unique_ptr identity) with a per-level Name() uniqueness check in GraphTransformerManager::Register.
  • Add a unit test validating: same name + same level is rejected, while same name + different level is allowed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
onnxruntime/core/optimizer/graph_transformer_mgr.cc Fixes duplicate detection by checking transformer Name() uniqueness per level before inserting.
onnxruntime/test/optimizer/graph_transform_test.cc Adds a regression test covering duplicate-name registration behavior across levels.

Comment thread onnxruntime/core/optimizer/graph_transformer_mgr.cc Outdated
Address review feedback that the per-level Name() scan in Register added quadratic cost. Reuse the existing transformers_info_ map (previously populated but unused for dedup) as an O(1) hash index keyed by (level, name), preserving per-level semantics: the same name may still be registered at different levels.
@GopalakrishnanN
GopalakrishnanN requested a review from xadupre July 24, 2026 00:37
@GopalakrishnanN
GopalakrishnanN merged commit b59a932 into main Jul 30, 2026
87 checks passed
@GopalakrishnanN
GopalakrishnanN deleted the GopalakrishnanN/graph-transformer-register-dedup branch July 30, 2026 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants