[shoumikhin]

What is wrong today

A QNN pass that adds nodes can give a new node the name of a model input, and export then fails with Node redefined name t_1!. This happens because torch.export does not record the names it assigns in the graph's name table, so a later pass can hand out a name that is already taken. Models with autocast regions hit it, such as robot policies exported in float16.

What this change does

Before its passes run, the QNN pass manager records the names of the existing nodes, so new nodes always get unused names. This fixes every pass in the pipeline at once, including the attention (SDPA) pass, rather than one place that creates nodes. torch.fx has no public way to reserve a name, so this writes the namespace's private name set. A TODO marks it for removal once torch.export records the names itself.

The to-edge passes need nothing: they run on the program that to_edge rebuilds, where every name is already recorded. Names inside nested autocast regions are already recorded too, so only the top-level graph needs this.

The test-only commits make the constant-only partition test check, in the lowered graph, that the whole constant branch stays on the CPU and that the rest is delegated.

Tests

On Linux x86_64, with torch 2.14 and a torch 2.16 nightly:

• The node name test (inputs named t_1 and mul) fails without the fix and passes with it. It lints the graph the pipeline returns, since ep.module() would rename duplicates. It also checks that torch.export left the input name unrecorded, so once torch.export records names the test fails with a message saying the workaround can go, and that the autocast and attention decompositions ran.
• test_passes.py passes, except test_mha_to_sha, which needs a torchao module that this environment does not have.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24316

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit e5f40e2 with merge base 7586c16 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 19m 11s —— View job

---

The reservation fix is correct for QNN's export pipeline, but it patches an upstream torch.export bug by writing private torch.fx state with no TODO, and both changed tests check less than they claim.

Detailed review

PR Review: #24316

Summary

QnnPassManager._transform now reserves every existing node name in the top-level graph's FX namespace, so nodes added by passes can't take a name torch.export gave a placeholder. A second commit tightens the constant-only partition test. The fix resolves the reported Node redefined name failures. The problems:

• It works around a torch.export bug inside one backend by writing private torch.fx state, and it has no exit plan.
• The two tests assert less than their names and commit messages say.

This review did not run the QNN tests (there is no QNN SDK on the runner). Claims about torch internals come from reading code, not from running it.

Code Quality

• backends/qualcomm/_passes/qnn_pass_manager.py:103-108: the workaround writes private torch.fx state in one backend, with no TODO
  • Root cause: in torch.export. placeholder_naming_pass assigns node.name directly and never registers the name.
  • Private state: the fix writes graph._graph_namespace._used_names, two levels of private torch.fx state. The other two uses in the repo only read it (backends/transforms/fuse_rms_norm.py:375, backends/transforms/fuse_batch_norm_with_conv.py:119).
  • Consequence: if a torch pin bump changes _Namespace, every QNN annotation, export and preprocess pipeline fails (lines 470, 481, 520), not just models that would collide.
  • Same bug elsewhere: EnnPassManager._transform (backends/samsung/_passes/enn_pass_manager.py:46-47) is identical to QNN's pre-fix body. Its transform_for_export_pass (:60-64) runs DecomposeRoll and DecomposeRemainder in place on a raw torch.export.export(...) program (backends/samsung/utils/export_utils.py:76-77). A model with an input named after a node those passes insert (e.g. mul) would likely fail the same way. I did not reproduce this.
  • Ask: file the upstream issue and add a TODO(<link>) at line 103.
    • The TODO should say this is a private-API workaround. The commit message says it ("torch.fx has no public way to reserve a name"); the code comment and PR description don't.
    • Optionally, move the reservation into a shared helper (e.g. backends/transforms/utils.py) that both pass managers call. Fix this →
• Low confidence: qnn_pass_manager.py:107 reserves names only in the top-level graph
  • Gap: when an outer autocast region is disabled, DecomposeWrapWithAutocast._replace(submod) (decompose_wrap_with_autocast.py:71) inlines nested regions into the submodule's own graph. This loop never touches that graph's namespace.
  • Condition: this matters only if the pinned torch also assigns placeholder names directly inside autocast subgraphs. I couldn't check the torch source here.
  • Fix: iterating every GraphModule in graph_module.modules() covers it at little cost. Fix this →

Testing

• backends/qualcomm/tests/test_passes.py:215-217: the new test never checks that names are unique
  • Why assert_close can't catch it: ep.module() deep-copies the graph, and the copy renames duplicate names.
  • Why it fails without the fix today: DecomposeWrapWithAutocast (decompose_wrap_with_autocast.py:94) and the SDPA decomposition (backends/transforms/decompose_sdpa.py:47) happen to run DCE, which lints the graph. A collision created by any pass that doesn't run DCE afterwards would pass this test.
  • Ask:
    • Call ep.graph_module.graph.lint() after transform_for_export_pipeline.
    • Before running the pipeline, assert that the placeholders really are named t_1 / mul. This follows the file's "Guard against a vacuous test" pattern (:926). Fix this →
• test_passes.py:1003-1004: the constant-only partition check is weaker than its commit message
  • Claim vs. check: the commit message says all of table * 2 - 1 must stay top-level and the add must be delegated. The test only asserts mul.Tensor and that some delegate exists.
  • Missed regression: a partitioner whose constant-only check is non-transitive would keep mul (reads only table) on CPU. It would then delegate sub (reads mul's output) as a separate partition that reads nothing at runtime. The test would still pass.
  • Ask: also assert that the sub is in top_level_ops and that exir_ops.edge.aten.add.Tensor is not. Fix this →

Recommendation

Approve

Nothing blocks. The reservation fixes the reported export-pipeline failures. Add the upstream TODO before merging. The shared-helper move and the test hardening can come later.

[shoumikhin]

@haowhsu-quic @shewu-quic @winskuo-quic could you take a look? This changes the Qualcomm backend.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 20m 42s —— View job

---

The node name reservation fixes the reported Node redefined name export failures and nothing blocks merging, but the TODO still links no upstream issue, the private-state write has a public alternative, and the new test can pass without exercising the fix.

Detailed review

PR Review: #24316

Summary

QnnPassManager._transform now adds every existing top-level node name to the graph's FX namespace before the passes run. Nodes the passes add therefore can't reuse names that torch.export assigned directly. Two test-only commits also tighten the constant-only partition test.

All findings are minor:

• The TODO gives no way to tell when the workaround can be removed.
• A public graph rebuild would avoid the private write.
• The workaround exists only in the QNN backend.
• Nested autocast graphs may not be covered (low confidence).
• The new test's guard checks only part of what the test depends on.

This review could not run Python or the QNN tests on this runner. Claims about torch internals come from reading code, not from running it.

Code Quality

• backends/qualcomm/_passes/qnn_pass_manager.py:103-109: the TODO links no upstream issue
  • "Remove once torch.export records the names it assigns" names no pytorch/pytorch issue, so nothing will signal when the workaround can go.
  • The comment is seven lines over a three-line loop, and the first block and the TODO both explain the cause.
  • Ask: replace it with one TODO(<issue link>) of two or three lines. Fix this →
• qnn_pass_manager.py:110-112: a public API does what the private write does
  • torch.fx has no public way to reserve a name. But graph_module.graph = copy.deepcopy(graph_module.graph) rebuilds the graph through Graph.graph_copy → create_node, which records every name.
  • This is the same copy that makes ep.module() rename duplicates, as the test comment at test_passes.py:222 notes.
  • Why it matters:
    • This is the repo's first write to _graph_namespace._used_names. The two other uses, backends/transforms/fuse_rms_norm.py:375 and fuse_batch_norm_with_conv.py:119, only read it.
    • _transform feeds the annotation, export and preprocess pipelines (lines 474, 485, 524). A torch pin bump that changes _Namespace would break all three at once.
  • Caveat: the rebuild replaces every Node object, so no code may hold node references across _transform. This review did not run it. Fix this →
• qnn_pass_manager.py:102-113: the workaround exists only in QNN, but the bug comes from torch.export's output
  • EnnPassManager._transform (backends/samsung/_passes/enn_pass_manager.py:46-47) is the same as QNN's body before this fix.
  • Its transform_for_export_pass (:60-65) runs passes that add nodes (ComputeConstAttrs, DecomposeRoll, DecomposeRemainder). They modify a raw torch.export.export(...) program in place (backends/samsung/utils/export_utils.py:76-77), so the Samsung backend is likely exposed the same way. This review did not reproduce it.
  • Ask: add a shared helper in backends/transforms/utils.py and call it from both pass managers. That also leaves one place to delete when the TODO is resolved. Fix this →
• Low confidence: qnn_pass_manager.py:110-112 reserves names only in the top-level graph
  • How it would fail:
    1. For a disabled autocast region, DecomposeWrapWithAutocast._replace(submod) (decompose_wrap_with_autocast.py:71) inlines nested enabled regions into the submodule's own graph, whose names were never reserved.
    2. graph.erase_node(input_submod) (:90) leaves that submodule attached to the module.
    3. dead_code_elimination_pass (exir/passes/__init__.py:502-505) runs eliminate_dead_code(), which lints, on every GraphModule in modules(). A duplicate name in the submodule raises the same Node redefined name error.
  • Condition: this only happens if the pinned torch also assigns names directly inside wrap_with_autocast subgraphs, for example if _name_hoo_subgraph_placeholders handles that HOP. The torch source was not available here to check.
  • Fix: reserve names in every GraphModule in graph_module.modules(). Fix this →

Testing

• backends/qualcomm/tests/test_passes.py:216-220: the guard against a vacuous test only checks that an input has the colliding name
  • The test also relies on two conditions it never asserts:
    • torch.export does not record that name in the namespace;
    • the pipeline still decomposes the autocast HOP or SDPA node, which is what creates the node that would take the name.
  • If either changes, both subtests pass without exercising the reservation, and nothing flags the workaround as dead.
  • Ask:
    • Before the pipeline runs, assert name not in ep.graph._graph_namespace._used_names. This assertion also starts failing once torch.export records the names, which tells you the TODO can be resolved.
    • After the pipeline, assert that no WrapWithAutocast or aten.scaled_dot_product_attention.default node is left, as lines 181-188 do. Fix this →

Recommendation

Approve

Nothing blocks: the reservation fixes the reported export failures. Link the upstream issue in the TODO before merging. The other changes can come later.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.