[shoumikhin]

What is wrong today

On HTP, a partition that reads nothing at runtime stays on the CPU, since HTP refuses such a graph. So a constant-only op that QNN keeps whole can land there undecomposed, and lowering fails:

RuntimeError: Node aten_linear_default ... was not decomposed or delegated.

What this change does

The HTP filter of ops_to_not_decompose now estimates which ops HTP will leave on the CPU, and decomposes the kept-whole ones.

• An op is a CPU op when its data types or shapes rule QNN out, or when it has no tensor output.
• A kept-whole op is left on the CPU when it reaches a QNN op through a CPU op before any op that reads a runtime value, since the partitioner may split it off.
• When only getitems of CPU ops read a runtime value, or such a getitem comes after a QNN op, it is also left on the CPU if no chain of QNN ops links it to such a reader.
• An op that reads dequantized values is never decomposed: if the estimate is wrong, QNN rejects its unencoded pieces.

The estimate can also decompose an op that a partition reading a runtime value still takes, where main keeps it whole.

Still failing as before: a kept-whole op that reads dequantized values or is isolated only by a skip or an op QNN rejects or removes, ops that QNN's passes rebuild after decomposition (rms_norm, pixel_unshuffle), the scatter variants, and ops inside control-flow bodies.

uint8 to float32 casts

On the HTP simulator, a uint8 to float32 cast turned every input above 0 into 1. The cast builder now sends an unquantized uint8 to float32 cast through int32, as it does for int64. GPU gets the step too, untested.

Follow-ups from the review of #24316

• The name collision test skips only when no case collides, and keeps the original error.
• A torch-only test checks reserve_node_names directly.

Tests

• test_htp_decomposes_kept_whole_ops_left_on_cpu covers each rule above and fails when any rule is removed.
• test_htp_kept_whole_estimate_avoids_quadratic_steps counts work on 2 graph sizes.
• test_qnn_backend_cast covers uint8 in floating point and quantized mode.
• All pass with torch 2.16 nightly on Linux x86_64, and the 2 HTP tests also with torch 2.14. With the real SDK, 4 quantized models stay fully delegated, as on main.

cc @cbilgin @psiddh

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 1 New Failure, 6 Pending, 1 Unrelated Failure

As of commit 3ae7c6e with merge base 18e0cc0 ():

NEW FAILURE - The following job has failed:

• Test QNN Windows MSVC build / build-qnn-windows-arm64 (gh)

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / nxp-mcuxpresso-test / linux-job (gh) (trunk failure)
  undefined reference to SYS_INFO_PrintAll'`

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. Try requesting a review at a lower effort level.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 34m 51s —— View job

---

This PR makes the HTP ops_to_not_decompose filter decompose kept-whole ops that a pre-decomposition run of the drop rule would leave on CPU. I'm requesting changes for three reasons: that estimate can also decompose ops that main delegates whole, the new exir Partitioner docstring misstates exir's behavior, and the rewritten test removes the only real-HTP CI coverage of the drop rule.

Detailed review

PR Review: #24429

Summary

On HTP, QnnPartitioner.ops_to_not_decompose now partitions the program before the kept-whole ops are decomposed, and decomposes the ones that land in partitions with no runtime input. The PR also documents the two call stages on exir's Partitioner. Problems found:

• The estimate can decompose more than it should.
• The base-class docstring is wrong in places.
• The test no longer runs against a real SDK and leaves several claimed cases untested.

All four changed files were reviewed.

Backward Compatibility

• The estimate can decompose kept-whole ops that main delegates whole (backends/qualcomm/partition/qnn_partitioner.py:419-432, with is_runtime_input at :325-328).
  • Why it happens: the drop rule counts any input that is not a parameter as a runtime input, including the output of an op running on CPU. The estimate ignores skips and every QnnOperatorSupport rejection (no visitor, to_be_implemented_operator, QCOM_FALLBACK_NODE, IsNodeSupportedByBackend).
    • In the estimate, a skipped or rejected feeder that reads only constants gets merged into the kept-whole op's partition. That partition then reads nothing at runtime, so the op is decomposed.
    • In the real partitioning, the feeder runs on CPU, its output counts as a runtime input, and the op's partition is delegated.
  • Trigger (fp16 HTP): x + F.linear(self.table.abs(), self.weight).to(torch.bfloat16).float() with skip_node_op_set={"aten.abs.default"}.
    • On main, {linear} reads the output of abs on CPU and is delegated whole.
    • With this PR, the estimate sees {abs, linear} fed only by buffers, so linear is decomposed and its pieces are delegated.
  • Consequences:
    • The description's claim that "every other kept-whole op stays whole" does not hold.
    • Node names after the decomposed op shift relative to main. A skip_node_id_set that works on main can then silently name a different node, which is the hazard the PR gives for leaving skips out of the estimate.
    • By the PR's own reasoning, the delegated pieces may lose the op's quantization encodings. I have not verified this.
  • The trigger is narrow, but the comment at :413-418 and the "Not covered" list only describe the opposite direction. At minimum, document this direction too and pin it with a test. Fix this →

API Design

• The new Partitioner.ops_to_not_decompose docstring misstates exir, and turns temporary exir internals into a contract for every backend (exir/backend/partitioner.py:108-121).
  • :120-121: "so those entries are never kept whole" is wrong.
    • _remove_invalid_ops_for_not_decompose allow-lists torchao.choose_qparams_affine.default, even though it aliases an output (exir/program/_program.py:1063-1074).
    • Blocklisted ops with no default decomposition (add/sub/mul/div.Tensor) stay whole anyway.
    • exir's own log wording is the accurate one: the request "is ignored", meaning the op is neither preserved nor checked after lowering.
  • :117-118: the post-lowering exemption covers every edge variant in _QUANT_PRIMITIVES (_program.py:1018-1024), not only the quantize and dequantize ops. That includes convert_element_type.no_fuse and choose_qparams.tensor (exir/operator/util.py:47-66).
  • Scope: naming a private symbol and the drop rules in the public base class locks in code that exir itself calls "a band-aid" and "a temp fix" (_program.py:1134-1139).
    • QNN relies only on the paragraphs about the two stages, ep.dialect, and the filter applying to the same ep. Consider keeping only those.
    • Every backend reads this contract, so this hunk should get sign-off from the exir owners. Fix this →

Testing

• The rewritten test always mocks HTP, which drops the only real-HTP CI coverage of the drop rule (backends/qualcomm/tests/test_passes.py:983-994).
  • What CI loses:
    • The only CI job that runs this file, test-qnn-passes-linux (.github/workflows/pull.yml:1290-1323), uses the QNN SDK image with QNN_SDK_ROOT set. On main, this test ran the real op-support check and the real HTP compile there.
    • Now IsNodeSupportedByBackend returns a truthy MagicMock, and preprocess_multimethod returns fake bytes. CI can no longer catch HTP refusing a partition that the drop rule keeps.
    • The original scenario, a constant-only subgraph cut off by a skipped relu, is no longer tested.
    • The neighbouring partitioner tests (:889, :1041, :1109) run a real lowering and call skipTest when the backend can't initialize.
  • The comment "Runs without the QNN SDK" (:983) is inaccurate. to_edge_transform_and_lower_to_qnn calls setup_qnn_sdk() itself (backends/qualcomm/utils/utils.py:418), and the test does not patch it. On Linux x86_64 with QNN_SDK_ROOT unset, the test downloads the SDK, or raises "Failed to set up QNN SDK" when offline (backends/qualcomm/utils/qnn_sdk_setup.py:68-110).
  • Fix: keep the original case as a real-SDK test. In the mocked test, also patch executorch.backends.qualcomm.utils.utils.setup_qnn_sdk, the way test_qnn_delegate.py:12349 patches it for qnn_manager_lifecycle. Fix this →
• Several claims in the PR description have no test (test_passes.py:951-1039):
  • Data-dependent shapes: the description lists cut-offs by data-dependent shapes as covered, but no test has one. test_partitioner_falls_back_on_data_dependent_shape (:1109) indexes the runtime input x, so it never forms a partition with only constant inputs.
  • Quantization: there is no quantized case, although keeping encodings is the stated reason for not decomposing everything.
    • In a quantized ATen graph, the estimate relies on _unsupported_dtypes_are_quantized.
    • The bf16 test next to this one already loops over 8a8w, fp16 and weight-only (:1053-1057).
  • assertIs(keep_whole, filter_fn) (:1023) checks function identity rather than the behavior its comment describes. A direct check would lower a constant-only scatter_add cut off by bf16 casts and assert that it still raises "was not decomposed or delegated". scatter_add works for this because it is kept whole and has no default decomposition.

Code Quality

• The filter silently depends on the order of exir's calls (qnn_partitioner.py:411-439).
  • How it works now: left_on_cpu() is computed lazily over the captured ep and matched by node identity. The filter exir actually uses comes from the call at exir/program/_program.py:911. That call runs on the program produced by the first run_decompositions at :1237, which the comments at :413 and :434 describe as "before decomposition".
  • Failure mode: if a node from any other program reaches the filter, every lookup misses. The filter then quietly becomes plain filter_fn, and the original error comes back with no hint why. One plausible way this happens is an exir cleanup that reuses the result fetched at :1193.
  • Fix: the new docstring states the contract, but nothing enforces it. Raising on a node that is outside ep.graph_module and its control-flow submodules would make a violation loud.
  • Comment at :406-407: it inverts the meaning at the EDGE stage. There, True marks a node that must not stay on CPU, and filter_fn does let must_stay_on_cpu nodes stay. Fix this →
• Kept-whole ops inside control-flow bodies are not handled, and the description doesn't list them as a gap (qnn_partitioner.py:419-425).
  • Why they fail: the estimate, like generate_partitions (:271-274), partitions only the top-level graph. exir also filters and checks the nodes of control-flow submodules (_program.py:931-940, 1034-1049). So a kept-whole op inside a torch.cond branch still fails on HTP.
  • Fix: QNN never tags submodule nodes, so adding every node from get_control_flow_submodules(ep.graph_module) to left_on_cpu() closes this cheaply.
  • Otherwise: add control flow to "Not covered". Add matmul there too: the opt-in ConvertBmmToMatmul pass (_passes/qnn_pass_manager.py:135) rebuilds it after decomposition, the same kind of failure as the rms_norm/pixel_unshuffle cases. Fix this →

Recommendation

Request Changes

What blocks approval:

• The wrong statements in the docstring need fixing.
• The lost real-HTP test coverage needs restoring.
• The over-decomposition side of the estimate should at least be documented and tested, if not fixed.
• The exir docstring hunk also needs sign-off from an exir owner.

---

[shoumikhin]

Thanks for the review. The latest push changes this:

• Decomposing too much: you are right that the estimate can also decompose a kept-whole op that ends up delegated, when a skipped or rejected op that reads only constants feeds it. The code comment and the description now say so. I did not add a test for it, because it would lock in a known gap, not a wanted behavior. Fixing it needs the real partitions before decomposition, which is a bigger change.
• Docstring: trimmed. It now keeps only the two stages, ep.dialect, and the rule that a filter applies to the ep of the same call. The drop rules and _QUANT_PRIMITIVES are gone. An exir owner should still look at this part.
• Real SDK: test_partitioner_keeps_constant_only_subgraph_on_cpu is back as it is on main, so CI runs it against the real SDK again. The stubbed checks are a separate test, which also patches setup_qnn_sdk, so it no longer tries to download the SDK.
• Data-dependent shapes: the stubbed test now has a constant-only linear cut off by a masked select.
• Quantized case: not added. For quantized graphs the estimate uses the same must_stay_on_cpu check as filter_fn. With the real SDK, Swin-v2-T in 8a8w lowers into one partition with nothing on the CPU, the same as on main.
• assertIs(keep_whole, filter_fn): kept. On the lowered program the contract is exactly "return plain filter_fn". A scatter_add test would also depend on scatter_add having no default decomposition, which can change.
• Nodes from another program: no check added. If that ever happens, the lookup misses, the op stays whole, and exir's check after lowering fails with "was not decomposed or delegated", the same error as today. So it cannot go wrong silently.
• Comments: reworded. The lowered-program comment no longer inverts the filter's meaning, and the comments and the description now say the estimate runs before the kept-whole ops are decomposed, not "before decomposition".
• Control flow: added to "Not covered". The estimate only sees the top-level graph, so ops inside control-flow bodies get plain filter_fn, as on main. A matmul rebuilt by ConvertBmmToMatmul falls under the "ops that QNN's own passes rebuild" item.

[shoumikhin]

@claude /pr-review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[claude]

Claude finished @shoumikhin's task in 26m 55s —— View job

---

I recommend approval: I found no correctness problems in this revision, and two minor requests remain: run the new scenario against the real QNN SDK in CI, and keep the HTP-only drop rule in one place.

Detailed review

PR Review: #24429

Summary

On HTP, QnnPartitioner.ops_to_not_decompose now decomposes kept-whole ops that the drop rule would leave on CPU. It estimates this on the largest possible partitions, before those ops are decomposed. The PR also updates exir's Partitioner docstring to describe the two stages at which exir calls this method.

I found no correctness problems beyond the limits the description already lists. Two minor problems remain:

• The fix is tested only with HTP stubbed out.
• The rule for which backends drop partitions without runtime inputs is now written in two places.

All four changed files were reviewed.

Code Quality

• The HTP-only drop rule is now written in two places (backends/qualcomm/partition/qnn_partitioner.py:409, :431).
  • Where: generate_partitions runs _drop_partitions_without_runtime_inputs only on HTP (:276-281; the comment there says why LPAI and GPU are left out). ops_to_not_decompose repeats the backend check at :409 and calls the drop rule itself at :431.
  • Why it matters: nothing keeps the two in step. The new test only builds HTP specs, so no test notices if they drift:
    • If the drop at :278 is extended to another backend but :409 is not, that backend fails again with "was not decomposed or delegated".
    • If :409 is loosened, GPU starts decomposing constant-only kept-whole ops that it delegates whole today, because GPU accepts graphs without inputs.
  • Fix: decide once whether the backend drops these partitions and use that in both places. One option is an attribute set in __init__ next to self.backend, with the comment from :276-277 moved there. Fix this →

Testing

• The fix only runs with HTP stubbed out, although the CI job for this file has the real SDK (backends/qualcomm/tests/test_passes.py:1014-1027).
  • What the stubs hide: get_or_create_qnn_manager returns a MagicMock. As a result, IsNodeSupportedByBackend (qnn_partitioner.py:217-222) accepts every op that reaches it, and the stubbed preprocess_multimethod compiles nothing. The test cannot see real HTP reject, or fail to compile, anything the estimate counts as delegated.
    • Example: the test expects the constant-only linear(self.table, self.weight) to stay whole inside the delegated partition. If real HTP refused that linear, it would stay whole on CPU and lowering would fail with the original error. This test would still pass.
  • Why existing coverage doesn't catch it:
    • test-qnn-passes-linux (.github/workflows/pull.yml:1290-1323) builds the SDK and runs this file.
    • No test in this file that runs against the real SDK has a kept-whole op cut off from the inputs. test_partitioner_keeps_constant_only_subgraph_on_cpu (:951-977) has no kept-whole op. The kept-whole ops in the bfloat16 and data-dependent-shape tests (:1074, :1142) read x.
  • Fix: keep the stubbed test, and also lower ConstantBranches without the patches, under the file's "Failed to initialize QNN backend" → skipTest guard (:966-976). Assert the same top-level and delegated ops, then call to_executorch(). Fix this →

Recommendation

Approve

Nothing I found blocks approval. The real-SDK test is the more important of the two requests, because it is the only way CI can catch the estimate disagreeing with the real HTP checks. The exir/backend/partitioner.py change still needs review from its code owner.

[shoumikhin]

Pushed 4 new commits, rebased on main:

• To pick which kept-whole ops to decompose, the HTP filter used to run the partitioner on the ATen graph. The partitioner's time grows with the square of the graph size, and on a model with about 43,000 nodes the export sat there for more than 2 hours. The estimate is now a few passes over the graph and takes under 4 s on that model. New test cases cover linears that the partitioner splits off to avoid a cycle, and a model whose input reaches only a CPU op.
• The 3 follow-ups from the review of Qualcomm AI Engine Direct - Keep node names unique across the QNN and Samsung passes #24316.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 56m 24s —— View job

---

Requesting changes: the PR's new HTP estimate of which kept-whole ops end up on CPU counts export's runtime-assert nodes as QNN ops, so a masked select makes it decompose linears that main delegates whole, and its cost grows quadratically with the number of constants.

Detailed review

PR Review: #24429

Summary

The PR adds an HTP-only estimate to QnnPartitioner.ops_to_not_decompose so that exir decomposes the kept-whole ops that _drop_partitions_without_runtime_inputs would leave on CPU, and adds tests and a docstring covering the two exir stages of Partitioner.ops_to_not_decompose. The estimate gets wrong the data-dependent case it targets, and it is not linear-time as the description says; the other findings are minor.

Code Quality

• The estimate is a 55-line closure, unlike the class's other graph analyses (backends/qualcomm/partition/qnn_partitioner.py:414-469).

  • _drop_orphaned_parameter_dequants (:284) and _drop_partitions_without_runtime_inputs (:323) are static methods that take the program. This one is folded into the stage handling of ops_to_not_decompose.
  • As a result, tests can only see its result through a full lowering, or through the returned filter, where it is mixed with filter_fn.
  • Fix: move the body into a @staticmethod that takes ep and returns the set, and keep only the functools.cache wrapper in ops_to_not_decompose. Tests can then check the set directly.

  Fix this →

Testing

• The setup_qnn_sdk patch doesn't cover building the compiler specs, so both new HTP tests still set up the real SDK (backends/qualcomm/tests/test_passes.py:1115-1123, :1192-1195).

  • generate_qnn_executorch_compiler_spec calls setup_qnn_sdk() itself (backends/qualcomm/utils/utils.py:1283).
  • The first test calls it before entering the with patch(...) block. The timing test patches nothing.
  • On Linux x86_64 with QNN_SDK_ROOT unset, both tests try to download the SDK and raise "Failed to set up QNN SDK" if that fails (backends/qualcomm/utils/qnn_sdk_setup.py:55-110). That contradicts the comment at :1120. CI has the SDK, so it doesn't catch this.
  • Fix: build compiler_specs inside the with block, and wrap the timing test's spec generation in the same patch.

  Fix this →

• test_reserve_node_names never checks that its premise holds (backends/transforms/test/test_reserve_node_names.py:19-25).

  • It checks that "mul_1" is a node name, but not that the namespace is missing that name.
  • Once torch.export does not record the node names it assigns, so a later pass can create a node with a name that is already used pytorch#202927 is fixed, create_node(name="mul_1") gets a fresh name without the helper. The test then keeps passing without exercising reserve_node_names, and never signals that the TODO at backends/transforms/utils.py:67-68 can be resolved.
  • The sibling QNN name collision test skips in that situation. Fix: do the same here, calling skipTest when "mul_1" is already in graph._graph_namespace._used_names before the helper runs.

  Fix this →

API Design

• The EDGE-stage bullet promises an error that exir skips for ops it does not preserve (exir/backend/partitioner.py:114-117).

  • When a partitioner returns a filter, exir drops mutable, aliasing and blocklisted ops from its list before the check (exir/program/_program.py:1426-1427, blocklist at :1107-1121). For example, a listed add.Tensor left on CPU raises nothing.
  • Fix: add "ops exir does not preserve" next to "some quantization ops" in the exceptions. That keeps the bullet accurate without naming exir internals.

  Fix this →

Backward Compatibility

• Nodes without a tensor output count as QNN ops, so a masked select can make the estimate decompose ops that main delegates whole (backends/qualcomm/partition/qnn_partitioner.py:435-440).

  • Cause: must_stay_on_cpu only inspects tensor values (backends/qualcomm/partition/utils.py:155-166, :193-199).
    • For a masked select's unbacked size, export inserts runtime-assert nodes (sym_constrain_range_for_size, _assert_scalar and the comparisons they check). These land in qnn.
    • HTP's export passes keep them in the ATen program the estimate reads.
    • None of them is ever delegated: the edge passes remove _assert_scalar and the dead comparisons (backends/qualcomm/_passes/remove_redundancy.py:34), and sym_constrain_range_for_size has no node visitor (qnn_partitioner.py:200).
  • Effect on the estimate: the CPU index appears to reach a QNN op through the CPU sym_size.int. So every constant-only op feeding that index lands in before_cpu (:452-457), which overrides linked (:465-469).
  • Trigger:

    def forward(self, x, mask):
        k = F.linear(self.table, self.weight)  # both buffers
        return x + k, k[mask].sum()

    • On main, {linear, add} is delegated as one partition. With this PR, the linear is decomposed and delegated in pieces. The mask can be an input or a buffer.
    • The added nodes can shift the names of later nodes of the same ops, which :427-428 and the SkippedFeeder test guard against. So a skip_node_id_set written against main can silently name a different node.
  • Fix: count as QNN only call_function nodes with a tensor in meta["val"], and add the trigger to the stubbed test.
    • Also excluding torch._ops.HigherOrderOperator targets closes one gap the comment at :429-433 accepts. Today, a constant-only linear feeding a top-level torch.cond still fails.

  Fix this →

Performance

• The estimate is quadratic in the number of constants, not linear as described (backends/qualcomm/partition/qnn_partitioner.py:444).

  • Cause: _is_runtime_input runs for every (QNN node, non-QNN input) pair until a runtime input turns up.
    • Each call goes through is_parameter and is_mutable_buffer_input (backends/qualcomm/builders/utils.py:39-47, :106-117).
    • Torch's ExportGraphSignature rebuilds inputs_to_parameters, inputs_to_buffers, inputs_to_lifted_tensor_constants and buffers_to_mutate from all specs on every access.
  • Cost: the dequantize of a PT2E-quantized weight reads only constants: the weight, plus scales and zero points when quantized per channel. So it never stops early.
    • A model with N such weights costs on the order of N² spec visits. The description claims "each visiting every node and edge once".
  • Test gap: test_htp_kept_whole_estimate_is_fast_on_large_graphs can't detect this, since Chain has no constants.
  • Fix: build the set of non-mutated constant placeholder names once per estimate from ep.graph_signature.input_specs, and test membership against it.
    • Give the timing test a few thousand constants that each feed an op, e.g. x = x + p * 2 over a torch.nn.ParameterList.

  Fix this →

Recommendation

Request Changes

Two problems block approval:

• The assert nodes make the estimate decompose ops that main delegates whole, in the data-dependent case this PR targets.
• The quadratic cost undercuts the reason for estimating instead of partitioning.

Both are small fixes inside left_on_cpu. The other findings are minor.

• Gather context
• Run three independent reviews (all six changed files covered)
• Consolidate findings
• Fact-check findings
• Post review

[psiddh]

@qti-horodnic can you take a look at this ?

[qti-horodnic]

@psiddh Yes, was planning to review this tomorrow

[shoumikhin]

Thanks. All items are addressed in the new head.

• Kept-whole ops that main delegates whole. You were right: the partitioner also merges ops that share no data, so the link rule decomposed the linear in your ConstantOutput model. The link rule now applies only when no op reads a runtime value, or when getitems of a CPU op do. That is wider than your suggestion, because such getitems can keep an op out of every partition that reads a runtime value even when other ops read one too, and on random graphs the narrower version left some kept-whole ops on the CPU. Both of your models are in the test. The first also returns a split of x, so getitems that QNN takes are covered. It keeps its linear whole, with the same node names as on main. The second lowers, while main fails on it. The cost: a model whose only runtime readers are skipped, rejected, or removed before partitioning, such as return x.clone(), linear * 2, fails again, as on main. The version you reviewed lowered it.
• Wall-clock test. It is now a count: on chains of 2 and 4 links, the estimate must never call the partitioner and must read the input specs of the graph signature as often on both. It fails on a version that runs the partitioner and on one that rebuilds the constant names for each input.
• Untested paths. The quantized test_qnn_backend_cast runs the uint8 model too, with a relative tolerance of 0.1, since the default of 1 would pass a cast that gives only 0 or 1. Its output is quantized, so it keeps the old one-step cast, which was already right on the simulator. The HTP test checks that GPU gets plain filter_fn on the ATen program.

[shoumikhin]

@claude /pr-review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[claude]

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

---

Request Changes: _left_on_cpu still decomposes a constant-only linear that main delegates whole, and neither the code nor the description mentions it; the tests also miss two parts of the rules and two of the regressions they are named for.

Detailed review

Summary

On HTP, ops_to_not_decompose now estimates, on the ATen program, which kept-whole ops HTP would leave on CPU and decomposes them. The cast builder now sends unquantized uint8 → float32 casts through int32. One rule of the estimate still changes the delegated graph of models that already lower on main. Two parts of the rules have no test, and two tests cannot catch the regression they are named for.

Backward Compatibility

• before_cpu decomposes kept-whole ops that main delegates whole (backends/qualcomm/partition/qnn_partitioner.py:427-437, the clause at 433). Take buffers table (4×16) and weight (16×16):

  def forward(self, x):
      r = x + 1
      lin = F.linear(self.table, self.weight)
      return lin.to(torch.bfloat16).float() + r.to(torch.bfloat16).float()

  • On main: the final add cannot join r or lin, because the casts would close a cycle. r and lin share no path, so they merge into one partition that reads x, and the linear is delegated whole.
  • With this PR: the linear's user is a CPU cast that reaches the final add. Line 433 puts the linear in before_cpu, _left_on_cpu returns {lin}, and the linear is decomposed into permute_copy + mm and delegated in pieces.
  • The rule cannot tell this apart from the detour branch of ConstantBranches. Both linears reach a QNN op through CPU casts, but here a partition that reads x can still take the linear.
  • Consequence: a model that lowers on main gets a different delegated graph. The new permute_copy and mm also shift the names of later nodes of those ops. A skip_node_id_set written against main can then silently skip a different node. That is the renaming that 405-406 avoid for skips.
  • Not documented: 403-404 state this cost for the link rule. 393-397 only say the partitioner "may" split such an op off, and the description lists only the cases that still fail.
  • Ask: if no linear-time check can separate this case from detour, say in 393-397 and in the description that such an op is decomposed although main delegates it whole.

  Fix this →

Testing

• Two parts of the rules have no test. The description says the HTP test "fails when any rule is removed".

  • Clause (a), qnn_partitioner.py:432: every linear that before_cpu catches in the test models feeds its CPU op directly, so deleting the clause changes no result. A model that returns x + F.linear(self.table, self.weight).relu().to(torch.bfloat16).float() fails on main, because {linear, relu} reads only constants and is dropped. This PR lowers it only because of line 432. It belongs in the loop at test_passes.py:1181.
  • The users direction of the link walk, line 446: no test model needs it. Take a model that does low, high = x.to(torch.bfloat16).split(8, dim=-1) and t = self.table * 2, then returns low, high, x + t, F.linear(t, self.weight). Main and this PR both delegate its linear whole. Without *node.users, the linear is decomposed. This model belongs in the kept_whole list at 1189-1194.

  Fix this →

• Two tests pass even with the regression they are named for.

  • SkippedFeeder (test_passes.py:1146, 1196-1204): suppose _left_on_cpu treated the skipped abs, unsqueeze, and add as CPU ops. The linear would then read the output of abs, which counts as a runtime value, so it stays whole as it does now. The test passes either way.
  • Fix for SkippedFeeder: compare the filters directly on a model where a skip does cut a kept-whole op off. For x + F.linear(self.table, self.weight).relu(), ops_to_not_decompose with and without skip_node_id_set={"aten_relu_default"} should agree on every node. Lowering that model with the skip fails, as the description says, so it cannot be tested by lowering.
  • test_htp_kept_whole_estimate_avoids_quadratic_steps: ops_to_not_decompose(ep) at 1281 runs before the with block. A version that computed the estimate eagerly would do all its work there. assert_not_called and both read counts (0 and 0) would then pass, even if that version ran the partitioner or rebuilt the constant names for each input. Moving line 1281 into the with block fixes this.

  Fix this →

Code Quality

• Low confidence: uint8 → float16 casts still take one step (backends/qualcomm/builders/op_to.py:114-118). The int32 step applies only when the output is float32. A uint8 → float16 cast still goes from UINT_8 to FLOAT_16 directly, for example in a .half() model fed uint8 images. If HTP's UINT_8 → FLOAT_16 cast has the same defect, it also returns only 0 or 1. I could not check this on the simulator. One run of a float16 variant of CastToFloat would settle it. If that run fails, test node.meta["val"].dtype.is_floating_point instead of comparing with float32.

  Fix this →

Recommendation

Request Changes

The before_cpu over-decomposition changes the delegated graph and the node names of models that lower on main, and neither the code nor the description says so. The tests do not support the description's claim that removing any rule makes them fail. Each fix is a comment, a test model, or a moved line.

---

• Gather context (7 commits, 9 files, +512/−31)
• Three independent reviews (all 9 files covered)
• Consolidate and dedupe findings
• Fact-check findings
• Post review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

Thanks. All items are addressed in the new head.

• A constant-only op with a CPU detour that a partition still takes. You are right, and I found no linear-time check that tells it apart from the detour case. Telling them apart needs to know which partitions can merge without a cycle, and that is the quadratic search this estimate avoids. So the code comment, the commit message and the description now say that such an op can be decomposed although a partition that reads a runtime value still takes it. This can also shift the names of later nodes. Ops that read dequantized values are now never decomposed, since QNN rejects their unencoded pieces, so quantized models are mostly unaffected.
• Tests. Your relu model is in the lowering loop; your shared-constant model, splitting x + 1, is kept whole, since the getitem case now needs a QNN op before the split. Each one fails once its clause is removed. The skip check now compares the filters with and without a skip of the relu op, on a model where that skip cuts the linear off. It fails on a version that treats skipped ops as CPU ops. SkippedFeeder is gone, since it could not catch that. The quadratic test now makes the filter inside the counted block, so work done up front is counted too.
• uint8 to float16. I ran a float16 version of CastToFloat on the HTP simulator. That cast is right without the int32 step: with the step removed, the float32 case fails and the float16 case still passes. So the step stays limited to float32 outputs.

[shoumikhin]

@claude /pr-review

[claude]

Reviewing PR #24429

• Gather context (7 commits on top of 18e0cc0, 9 files, +583/−32)
• Run three independent reviews (all 9 files covered)
• Consolidate findings
• Fact-check findings
• Post review

View job run

[shoumikhin]

@claude /pr-review

[claude]

Claude finished @shoumikhin's task in 55m 17s —— View job

---

I recommend approving, since nothing blocks; the ops_to_not_decompose docstring doesn't say that quantized programs still fail as on main, the quadratic-steps test can't catch the loss of functools.cache, and there are two minor items plus a non-blocking question about folding constants instead.

Detailed review

PR Review: #24429

Summary

On HTP, ops_to_not_decompose now estimates on the ATen program which kept-whole ops _drop_partitions_without_runtime_inputs would leave on CPU, and decomposes them. Unquantized uint8 → float32 casts now go through int32. Nothing blocks. The docstring overstates the fix for quantized programs, and one test misses the regression it was written for. There are also two minor items and a design question.

API Design

• Quantized programs still fail as on main, and the docstring doesn't say so (backends/qualcomm/partition/qnn_partitioner.py:540-542, 554-559).

  • The filter keeps every op that reads a dequantize output whole (559). In a QnnQuantizer program, a kept-whole op such as linear reads dequantize outputs, so for that op the filter is plain filter_fn, as on main.
  • Trigger: the masked branch of ConstantBranches (test_passes.py:1244), quantized and lowered with use_fp16=False. The linear's constant-only partition is still dropped, and exir still raises "was not decomposed or delegated". I traced this through the filter; I did not run it.
  • The docstring says these ops "are decomposed too", with no exception. The description lists the rule, but its "Still failing as before" list leaves out quantized programs.
  • The reason given at 554-555 holds only when the estimate is wrong. QNN can only reject pieces that are delegated to it, and when the estimate is right the pieces stay on CPU.
  • Ask: put the exception in the docstring and in "Still failing as before", and say in the comment that the exception guards against a wrong estimate.

  Fix this →

Testing

• The quadratic-steps test can't catch the loss of the cache (backends/qualcomm/tests/test_passes.py:1496-1517).

  • exir calls the filter on every node (exir/program/_program.py:921-922). Without functools.cache (qnn_partitioner.py:550), each call would rerun the linear-time estimate, for quadratic time overall.
  • The test calls keep_whole on one node (1513), so both sizes still read the input specs once and the test passes.
  • It also never reaches the lookup this PR changed in _drop_partitions_without_runtime_inputs (362-371), since it asserts that the partitioner never runs.
  • Ask: call keep_whole on every node, and require the same nonzero count at both sizes. Also count a direct call to the drop rule.

  Fix this →

• A failure in the HTP test doesn't name the model (backends/qualcomm/tests/test_passes.py:1396-1418, 1447-1456).

  • 11 models are lowered and checked in loops without self.subTest(model=type(model).__name__), which other tests in this file use (e.g. 194, 725).

  Fix this →

Code Quality

• The uint8 cast step runs on every QNN backend (backends/qualcomm/builders/op_to.py:100-118).

  • The comment says it works around an HTP defect, but builders don't know the backend, so GPU graphs also get the extra Cast to int32. Only the HTP simulator was checked.
  • Ask: say in the comment that the step applies to all backends, or check that GPU accepts it.

  Fix this →

• Design question (non-blocking): fold the constant-only subgraphs instead of estimating (backends/qualcomm/partition/qnn_partitioner.py:401-477).

  • Every op _left_on_cpu can return, and every partition the drop rule drops, computes only from parameters, unmutated buffers, and lifted constants.
  • Folding those subgraphs before partitioning would remove the failure exactly. There would be no estimate, no over-decomposition, and no renaming. It would also cover rms_norm, pixel_unshuffle, and scatter from "Still failing as before".
  • QNN already has the pieces. ConvertMhaToSha calls constant_prop_pass (_passes/convert_mha_to_sha.py:622), and to_edge_transform_and_lower_to_qnn runs QNN's edge transform passes before partitioning (utils/utils.py:525).
  • Trade-offs:
    • The .pte changes for models that lower today.
    • Folded tensors can be larger than their inputs.
    • constant_prop_pass skips quant primitives and control flow, so quantized programs and cond bodies would stay unfixed.

  Fix this →

Recommendation

Approve

Nothing blocks. Before merging, I'd fix the docstring and comment: as written, they promise quantized programs a fix they don't get. The exir/backend/partitioner.py docstring change still needs an exir code owner.

• Gather context (7 commits on top of 18e0cc0, 9 files, +583/−32)
• Run three independent reviews (all 9 files covered)
• Consolidate findings
• Fact-check findings
• Post review

[shoumikhin]

Thanks for the review.

• Folding constants instead. I agree it would cover more cases, but I would keep it out of this change. It changes the .pte of models that lower today, and a folded tensor can be larger than its inputs, as you note. It fits better as its own change, measured on real models.
• The other items (the docstring, comment and description for quantized programs, the quadratic-steps test, the model names in the HTP test, and the GPU note on the uint8 cast) will go into the next push, or into a follow-up if this merges first.

[shoumikhin]

@claude /pr-review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[claude]

Requesting changes for one regression: the reads_dequantized guard keeps a quantized consumer whole, but it still decomposes the kept-whole dequantize op that consumer reads. A constant-only op with LPBQ, affine or group-quantized weights can therefore lose its weight encoding in a model that main delegates whole.

Detailed review

PR Review: #24429

Summary

On HTP, ops_to_not_decompose now estimates, in linear time, which kept-whole ops _drop_partitions_without_runtime_inputs will leave on CPU, and lets exir decompose them. The drop rule now applies only to HTP, and unquantized uint8 → float32 casts go through int32. I found one regression, in the quantized guard, and two gaps in the tests.

Backward Compatibility

• The guard keeps a quantized consumer whole but still decomposes the kept-whole dequantize it reads (backends/qualcomm/partition/qnn_partitioner.py:558-561; the ops are listed at backends/qualcomm/partition/utils.py:286-289).
  • Mechanism:
    • A weight dequantize reads only constants and feeds its consumer. So whenever the consumer is in _left_on_cpu, the dequantize is too, in every branch of _left_on_cpu.
    • reads_dequantized is False for the dequantize itself, so the filter rejects torchao.dequantize_affine and dequantize_per_channel_group.
    • The second run_decompositions (exir/program/_program.py:1250) then splits the dequantize into integer and float arithmetic, while the consumer stays whole.
  • When it happens: this hits exactly where the guard was meant to help, when the estimate is wrong.
    • QuantizedCast (backends/qualcomm/tests/test_passes.py:1354-1359) is such a case. The quantization attributes don't exist yet when the filter runs. The quantize_per_tensor after the bfloat16 cast reads bfloat16 from an op that is not a dequantize, so must_stay_on_cpu holds for it (utils.py:169-180). The linear therefore lands in _left_on_cpu, although the whole model is delegated.
    • With the default 8a8w config, the weight dequantize is per-tensor and not in the skip table, so the test passes.
    • Some weights go through dequantize_affine: LPBQ (QuantDtype.use_16a4w_block), and torchao weight-only quantization, as in test_partitioner_falls_back_on_bfloat16 (:1547-1553). Main delegates that dequantize whole, together with the linear. With this PR, the linear's weight reaches QNN as unencoded arithmetic, without its per-block or per-channel encoding. That is the failure the comment at :555-557 describes for consumers.
    • I traced this by reading the code; I did not run it.
  • Fix: keep the kept-whole quantize/dequantize ops whole too, and compute the whole decision once, inside the cache, on the unmodified graph. _Q_OPS/_DQ_OPS would need to be imported from .utils, or folded into reads_dequantized.

    decomposed = functools.cache(
        lambda: {
            node
            for node in self._left_on_cpu(ep)
            if node.target not in _Q_OPS | _DQ_OPS and not reads_dequantized(node)
        }
    )

    • Calling reads_dequantized for each node, as the code does now, stops working once the dequantize is kept whole. exir rewrites each accepted node's target inside the same loop (exir/program/_program.py:921-929), before that node's consumer is filtered.
    • A weight-only consumer, whose only dequantized input is that kept-whole op, would then look unquantized and be decomposed.
    • Add an LPBQ variant of QuantizedCast to the test.
  • Fix this →

Testing

• The new scenarios still run only against a mocked HTP (backends/qualcomm/tests/test_passes.py:1367-1380). This is still open from the last round.
  • The mock accepts every op, which is the same assumption the estimate makes. Nothing checks that HTP's real op validation agrees with _may_run_on_qnn, or that it accepts the decomposed pieces.
  • test-qnn-passes-linux builds the SDK and runs this file (.github/workflows/pull.yml:1290-1323). So lower ConstantBranches for real, with the same skip as test_partitioner_keeps_constant_only_subgraph_on_cpu (:1202-1228).
  • Fix this →
• The mutated-buffer exclusion in _constant_inputs has no test (backends/qualcomm/partition/qnn_partitioner.py:340, :346).
  • The PR rewrites this check, which used to call is_mutable_buffer_input. The drop rule and the estimate now both depend on it.
  • Suggested test: a linear that reads a buffer the model mutates in place (e.g. after self.state.mul_(2)) and feeds a bfloat16 cast should stay whole. Asserting that would pin the exclusion.
  • Fix this →

Recommendation

Request Changes

The guard gap is a regression from main for constant-only ops whose weights are LPBQ, affine or group-quantized. The fix is a few lines plus one test model. The two test items are minor.

---

• Gather context (7 commits on top of 18e0cc0, 9 files, +595/−32)
• Run three independent reviews (all 9 files covered)
• Consolidate findings
• Fact-check findings
• Post review

View job run