Skip to content

Qualcomm AI Engine Direct - Add composite lowering for aten.convolution_backward - #23610

Merged
psiddh merged 3 commits into
pytorch:mainfrom
CodeLinaro:dev1/xiyushi/conv_backward
Nov 3, 2026
Merged

psiddh merged 3 commits into
pytorch:mainfrom
CodeLinaro:dev1/xiyushi/conv_backward

Conversation

@quic-xiyushi

@quic-xiyushi quic-xiyushi commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds Qualcomm AI Engine Direct composite lowering for aten.convolution_backward.default when only the input gradient is requested. This PR builds on work originally implemented by Rajeswaran C Ravindran @rajeswar277 and Mohit Gupta @quic-mohitgupta of the Qualcomm On-Device Learning team.

The lowering reuses the static filter wrapper shared with the forward Conv2d, emits the required runtime HWIO→HWOI QNN Transpose, and lowers the operation to QNN TransposeConv2d. This avoids requiring a separate duplicated backward weight.

This is implemented as a composite visitor rather than an ATen decomposition: the visitor must reuse the forward Conv2d QNN wrapper and retain the HWOI conversion as a runtime operation. An ATen decomposition would otherwise need another transformed static weight or changes to the forward Conv2d visitor.

Supported scope is HTP V69+, floating-point static 2D convolution filters with groups=1, transposed=False, dilation=[1, 1], and output_mask=[True, False, False]. The gradient input/output and filter must be rank-4.

Quantized and custom-dequant ConvBackward lowering is intentionally out of scope for this PR and will be added separately.

Test plan

  • Structural lowering:

    • python -m pytest -q backends/qualcomm/tests/test_passes.py -k "skip_decomp_table_preserves_conv_backward or conv_backward_composite_lowering or conv_backward_quantized_inputs_fallback or layout_transform_tuple_outputs"
    • Covers decomposition preservation, composite TransposeConv2d lowering and HWOI-wrapper reuse, quantized fallback, and synthetic tuple-layout edge cases (multiple outputs, None outputs, and malformed users).
  • HTP rework:

    • test_conv_backward passed on V68, V69, V73, V75, V79, and V81.
    • V68 verifies expected fallback; V69 through V81 verify successful delegation.
  • Rework pass coverage:

    • python -m pytest -q backends/qualcomm/tests/rework/passes/test.py -k "test_layout_transform or test_i64_to_i32"
    • Covers FP ConvBackward tuple-output layout placement through the preprocess pipeline, plus optional tuple metadata (grad_input, None, None).
  • On-device:

    • TestQNNFloatingPointOperator.test_qnn_backend_conv_backward passed on SM8850.
  • Tuple-output layout regression coverage:

    • Ran BatchNorm2d, MaxPool2d, GroupNorm, InstanceNorm2d, and AdaptiveMaxPool2d-with-indices on V69 and result matched clean latest main.

@quic-xiyushi
quic-xiyushi requested a review from psiddh as a code owner October 8, 2026 21:33
@pytorch-bot

pytorch-bot Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 2 New Failures, 1 Unrelated Failure

As of commit 907494a with merge base 14b5e1d (image):

NEW FAILURES - The following jobs have failed:

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

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

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

@meta-cla

meta-cla Bot commented Oct 8, 2026

Copy link
Copy Markdown

Hi @quic-xiyushi!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@quic-xiyushi

Copy link
Copy Markdown
Contributor Author

@pytorchbot label "release notes: qualcomm"

@pytorch-bot pytorch-bot Bot added the release notes: qualcomm Changes to the Qualcomm backend delegate label Oct 8, 2026
@psiddh

psiddh commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

@claude review this pr

@claude

claude Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Claude finished @psiddh's task in 5m 31s —— View job


PR Review: #23610

This PR adds a composite QNN lowering of aten.convolution_backward to TransposeConv2d for the input gradient only. Before merging, the V68 exclusion has to be enforced in code, and a quantized model needs to be shown to fall back cleanly.

Detailed review

Summary

The PR adds a composite QNN lowering of aten.convolution_backward.default (output_mask=[True, False, False]) to TransposeConv2d. It reuses the forward Conv2d's static weight wrapper and adds a runtime HWIO→HWOI Transpose. It also extends LayoutTransform and I64toI32 to handle tuple outputs with several getitems or with None entries.

I found no problems in the math (weight layout, output_padding derivation, the shared wrapper key) or in the I64toI32 changes. The problems are:

  • V68 is not gated. The PR says V68 can't compile this op, but nothing in the code stops the op from being delegated there.
  • The quantized-model guard is weak. It checks graph shape (is the producer a dq node) instead of quantization metadata.
  • Fallback paths have no tests.
  • There are a few smaller layout-pass and code-quality issues.

The diff was reviewed against merge base ff09af0d, because origin/main could not be fetched in this shallow checkout. No tests were run; all findings come from reading the code.

Infrastructure

  • V68 is excluded only by leaving it out of the test lists (backends/qualcomm/builders/op_conv_backward.py:25).
    • The docstring says "HTP V69+", and the PR description says the V68 SDK can't compile the composite. But neither ConvBackward nor QnnOperatorSupport.is_node_supported checks the HTP arch.
    • A user lowering for an SM8350/V68 target will see the node delegated, and then TransposeConv2d will fail to compile hard instead of falling back to CPU.
    • The partitioner already has this pattern at partition/qnn_partitioner.py:124-134 (the PER_CHANNEL_GROUP_ENCODING check with htp_arch < HtpArch.V69). Add exir_ops.edge.aten.convolution_backward.default to an equivalent gate.
    • The same gap applies to TestQNNFloatingPointOperator.test_qnn_backend_conv_backward (tests/test_qnn_delegate.py:607). It has no SoC or backend skip, so running the suite with --model SM8350 or a non-HTP backend will fail.
    • Fix this →

Code Quality

  • The quantized-input guards check graph structure, not quantization metadata (op_conv_backward.py:56, op_conv_backward.py:72).
    • The checks only ask whether args[0] or args[2] is produced by a dq op.
    • In a pt2e-quantized model, the forward conv's weight goes through a dq, but the backward still takes the raw parameter, so the args[2] check passes.
    • AnnotateQuantAttrs._annotate_source_nodes (_passes/annotate_quant_attrs.py:46-56) then writes QCOM_QUANT_ATTRS onto that shared parameter node. After FoldQDQ removes the q/dq around grad_output, the args[0] check passes too.
    • The result is that define_tensor(filter_node, ...) can return the forward conv's cached quantized weight wrapper. That wrapper then feeds a Transpose whose _hwoi output is declared float with QNN_QUANTIZATION_ENCODING_UNDEFINED, and then a float TransposeConv2d. That is either a compile failure or a silent mixed-precision op, which contradicts "quantized ConvBackward is out of scope".
    • Rejecting on QCOM_QUANT_ATTRS in grad_output_node.meta / filter_node.meta matches how the other visitors detect quantization. Medium confidence: I traced this by reading the code; I did not run a quantized model.
    • Fix this →
  • The unused forward input x (args[1]) is still layout-annotated (_passes/layout_transform.py:50).
    • Because convolution_backward is now in layout_sensitive_ops, traverse (layout_transform.py:302-308) walks every arg, so a permute(x) is inserted even though the visitor never reads x.
    • When x reaches the delegate only through the backward op (a backward-only graph, or the forward conv falls back), x becomes a needless delegate input and the QNN graph gets a Transpose whose output nothing consumes.
    • Consider making traverse skip args[1] for this target, or have the visitor define nothing for it and keep x out of the partition.
  • The new tuple path in insert_node puts the permute after a getitem that is already marked transformed (layout_transform.py:237-252). Low confidence; please confirm.
    • When the tuple producer is neither layout-sensitive nor layout-agnostic, traverse has already marked the getitem with QCOM_AXIS_ORDER (getitem is agnostic), and the permute is now inserted after that getitem.
    • op_transpose.define_node then calls get_tensor(getitem, getitem) (op_transpose.py:33-34). That returns an already-NHWC shape, and the NCHW→NHWC permute is applied a second time.
    • permute.meta[QCOM_QUANT_ATTRS] = layout_node.meta... (line 268) also copies the tuple producer's quant attrs instead of the getitem's own. annotate_quant_attrs.py:50-52 sets those per getitem, so a quantized permute after a getitem at index ≠ 0 would get output 0's encoding.
    • The cited regression coverage (BatchNorm2d, MaxPool2d, GroupNorm, InstanceNorm2d, AdaptiveMaxPool2d) is all layout-sensitive producers, so none of it exercises this branch.
  • mark_as_transformed still takes the axis order from the first getitem user only (layout_transform.py:168-179).
    • insert_node was generalized to handle every getitem, but this function was not, and it has no None guard either.
    • With several getitems of different ranks (for example a rank-1 grad_bias listed first), the producer is tagged with the wrong order. It is latent today only because the visitor rejects masks other than [True, False, False].
  • The comment contradicts the code (op_conv_backward.py:188-190). It says the weight is produced "by permute(2, 3, 0, 1)", but line 191 uses permute(2, 3, 1, 0), and the HWIO/HWOI naming in the comment is self-contradictory.
  • The arg-layout comment block (op_conv_backward.py:280-291) restates the ATen schema; per CLAUDE.md, drop it.
  • Unsupported paths log inconsistently (op_conv_backward.py:293-303). Only a None from _get_supported_input_nodes logs a second, generic warning; the other two helpers return silently after their own warning.
  • The padding length rules are inconsistent (op_conv_backward.py:127,148-149). padding may have length 1 while stride must have length 2. Edge-dialect convolution_backward always carries 2-element lists, so the expansion branch is dead; require length 2 for both and drop it.

Testing

  • No test covers the fallback paths.
    • Every model uses one configuration: k=3, stride 2, padding 1, groups 1, a static float weight.
    • Nothing checks that these variants are rejected by the partitioner and still produce correct output through the portable convolution_backward kernel:
      • output_mask with grad_weight/grad_bias
      • groups>1
      • dilation>1
      • transposed=True
      • a non-parameter weight
      • a quantized model
    • return_grad_weight=True is only used in a LayoutTransform structural test.
    • A quantized test that asserts fallback matters most, given the guard issue above.
  • The rework tests don't follow the neighbouring pattern (tests/rework/htp/op/v*/test.py). They hand-roll a single {"act": None} parametrization instead of using enumerate_activation_dtype, so there is no quantized variant, not even one that expects fallback.
  • test_skip_decomp_table_preserves_conv_backward only checks list membership (tests/test_passes.py:71-75). It does not show that export through the QNN flow actually keeps aten.convolution_backward.default; assert on an exported graph instead.

Performance

  • The weight may still be duplicated in the context binary (op_conv_backward.py:215-230). The HWIO→HWOI Transpose takes a STATIC tensor as input, and the HTP graph compiler may constant-fold transposes of static inputs, which would store a second, pre-transposed copy of the weight. That would undo the PR's stated goal. Low confidence: please confirm by comparing context-binary size with and without the backward op.

Recommendation

Request Changes

The blocker is the missing V68 / non-HTP gate: the PR itself says V68 can't compile this op, but it will still be delegated there. I'd also like to see the quantized-model guard tightened, and at least one fallback test added.

…on_backward

Co-authored-by: rajeswar277 <rajeswar@qti.qualcomm.com>
Co-authored-by: quic-mohitgupta <guptam@qti.qualcomm.com>
@psiddh

psiddh commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

@quic-xiyushi Can you look at the one blocker comment from Claude review ? looks reasonable to me

@quic-xiyushi
quic-xiyushi force-pushed the dev1/xiyushi/conv_backward branch from 94089b7 to 0adf1fb Compare October 27, 2026 22:45
@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: mohitguptams / name: Mohit Gupta (c846099)
  • ✅ login: quic-xiyushi / name: Xiyue Shi (2fa76f4, c846099)
  • ✅ login: rajeswar277 / name: rajeswar277 (c846099)

@quic-xiyushi
quic-xiyushi force-pushed the dev1/xiyushi/conv_backward branch from 0adf1fb to 2fa76f4 Compare October 27, 2026 23:02
@quic-xiyushi

Copy link
Copy Markdown
Contributor Author

@psiddh Thanks! I agree that Claude’s reviews are very helpful. I addressed the V68 blocker by adding an HTP V69 capability gate in QnnOperatorSupport, following the existing per-channel-group pattern. V68 now falls back before the QNN support query, and I added coverage for the V68 fallback. I also tightened the ConvBackward quantization guard to check QCOM_QUANT_ATTRS on grad_output and the shared weight, rather than only checking for direct dequantize producers.

@quic-xiyushi
quic-xiyushi force-pushed the dev1/xiyushi/conv_backward branch 3 times, most recently from d5f2f9d to 3687f8f Compare October 31, 2026 01:03
@quic-xiyushi
quic-xiyushi force-pushed the dev1/xiyushi/conv_backward branch from 3687f8f to d3030b5 Compare October 31, 2026 01:23
@psiddh
psiddh merged commit 18e0cc0 into pytorch:main Nov 3, 2026
215 of 219 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: qualcomm Changes to the Qualcomm backend delegate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants