Fix create_constant_placeholder crashing when torch.fx renames the requested placeholder

[slipstr34m]

Fixes #14055
Fixes #21541

Problem

Found while verifying #21489: the textbook CNN used there, one top-level nn.Sequential of conv, batchnorm, relu and maxpool, gets past the pooling check it used to crash on and now dies one pass later, in FuseBatchNormPass.

create_constant_placeholder records the name it requested, not the name torch.fx assigned:

node = graph.create_node(op="placeholder", name=name, target=name)
...
node_names = [n.name for n in graph.nodes if n.op == "placeholder"]
node_index = node_names.index(name)   # ValueError after any rename
...
arg_spec = TensorArgument(name)

torch.fx renames the node whenever the requested name is not a valid identifier or collides with an existing node. Two plain models trigger this today. A top-level nn.Sequential names its parameters 0.weight, so FuseBatchNormPass requests 0_weight_fused_bn, fx assigns _0_weight_fused_bn, and lowering to XNNPACK dies with ValueError: '0_weight_fused_bn' is not in list. torchvision regnet under Vulkan hits the same lines through its hyphenated module names (#14055). The identical models behind a named attribute lower fine, which is why existing coverage never reached this path.

Two further defects sit behind the crash. A placeholder's target is emitted verbatim as a function parameter name on recompile, so fixing only the lookup produces def forward(self, 0_weight_fused_bn, ...), a SyntaxError. And the shared-weight dedup from #18031 matched on target == name, so after a rename it misses and creates a duplicate placeholder.

Fix

One name everywhere: node.name, node.target, the state_dict key and the graph signature all follow the name fx assigned. The requested name is kept in node.meta and used for the dedup, so the #18031 contract is preserved: a second request for the same name returns the existing node, now also when fx renamed it. Keying the state_dict by the assigned name inherits the fx namespace uniqueness guarantee, so a colliding request can never overwrite an existing parameter.

For any requested name that is a valid identifier with no collision, the assigned name equals the requested name and behavior is unchanged.

Effect

Same script on either side of the change, executed on executor_runner built from this branch. maxdiff is deviation from eager.

                                     BEFORE                        AFTER
nn.Sequential(conv, bn, relu)        ValueError at utils.py:151    delegated, maxdiff 4.0e-07
two-block Sequential CNN             ValueError at utils.py:151    delegated, maxdiff 4.8e-07
same models behind an attribute      lower and run                 unchanged, maxdiff <= 5.5e-07

Testing

Two helper tests in test_create_delete_constant_placeholder.py: a digit-leading request checks node, signature, state_dict and a recompile round trip; a dedup test checks that requesting the same name twice returns the existing node for a clean, a digit-leading and a hyphenated name. One pass test in test_batch_norm_fusion.py fuses a top-level Sequential end to end. test_conv_bn in runtime/test/test_runtime_xnnpack.py, skipped on this bug, is re-enabled (linux-gated, runs in CI).

Verified failing-first by reverting only backends/transforms/utils.py: exactly the three new tests fail, all with ValueError at utils.py:151.

backends/transforms/test + backends/xnnpack/test/passes + runtime/test/test_runtime_xnnpack.py
  main    : 165 passed, 57 skipped
  this PR : 168 passed, 57 skipped

lintrunner reports no issues on all changed files.

The helper is shared by the Arm, Vulkan and XNNPACK backends. Callers passing valid unique names get identical behavior; renamed cases previously crashed, so no caller can depend on the old behavior.

cc @GregoryComer @digantdesai @cbilgin @JakeStevens

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 1 Awaiting Approval

As of commit 5197377 with merge base f287863 ():

AWAITING APPROVAL - The following workflow needs approval before CI can run:

• periodic (gh)

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

[slipstr34m]

@pytorchbot label "release notes: none"

[JakeStevens]

drop the issue number

[JakeStevens]

please make this comment more concise

[JakeStevens]

@claude please review

[claude]

Claude finished @JakeStevens's task in 1m 17s —— View job

---

Code Review

I read the three PR commits (HEAD~3..HEAD) against the current branch state. The two prior review comments (drop the issue number in test_batch_norm_fusion.py, tighten the utils.py comment) are already handled by commits c551269 and 5197377.

The fix is correct and well-targeted. The core insight — one name everywhere (node.name → node.target → state_dict key → TensorArgument), with the requested name preserved in meta for dedup — is the right shape. A few notes:

Correctness ✅

• backends/transforms/utils.py:122-123 — graph.create_node(name=name) lets fx assign the real name, then target = node.target = node.name binds target, state_dict key, and arg_spec (line 162) to the assigned name consistently. This is what makes the digit-leading / hyphenated cases lower instead of crashing at the old node_names.index(name).
• Keying state_dict/constants by target (the fx-assigned name) inherits fx's namespace-uniqueness guarantee, so a colliding request can't clobber an existing parameter. Good.
• The dedup loop (utils.py:112-116) now matches on meta["requested_name"] rather than target == name, so it correctly finds the existing node even after a rename — preserving the [ET-VK][ez] Fix duplicate placeholder target in create_constant_placeholder #18031 shared-weight contract.

Worth a thought (non-blocking)

• Dropping the old if name in state_dict or name in constants: guard means the dedup loop now scans all placeholders on every call, including the common no-rename path. It's the same O(placeholders) as before (that guard just short-circuited), so no regression — just noting the behavior is now "always scan."
• meta["requested_name"] is a create-time-only signal; it isn't expected to survive re-export/serialization. That's fine for the dedup's purpose (within a single pass session), but the contract is worth a one-liner if anyone later expects it to persist. Optional.
• Dedup matches on requested_name regardless of kind/data — a second request with the same name but different kind silently returns the first node. This matched the old behavior (which keyed only on name), so no change, but it's a latent sharp edge.

Tests ✅

• test_create_delete_sanitized_name covers the important path — the recompile round trip at exported_program.module()(torch.zeros(1)) == 1 is exactly what would have thrown SyntaxError on the verbatim target, so it guards the second defect described in the PR.
• test_create_same_name_returns_existing_node exercises clean, digit-leading, and hyphenated names against the dedup.
• Re-enabling test_conv_bn and adding the top-level-Sequential fusion test close the loop end to end.
• The failing-first verification (revert only utils.py → exactly the three new tests fail at utils.py:151) is a strong signal the tests actually pin the bug.

Nits (optional)

• In utils.py, target = node.target = node.name reads a little dense; node.target = node.name; target = node.name is marginally clearer, but this is fine as-is given the repo's terse style.

Nothing blocking from my side — the change is minimal, the name flow is now internally consistent, and the coverage is convincing. LGTM.
· fix-constant-placeholder-name

[JakeStevens]

create_mutable_buffer is similar, there may be a latent bug there too which can be resolved in follow up PR

[slipstr34m]

Sounds about right to me, will investigate and push a follow up PR later.