Skip to content

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

Open
quic-xiyushi wants to merge 2 commits into
pytorch:mainfrom
CodeLinaro:dev1/xiyushi/conv_backward
Open

quic-xiyushi wants to merge 2 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 forward convolution weight, emits the required QNN layout 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 static 2D non-transposed, non-dilated convolutions with output_mask=[True, False, False].

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

Test plan

  • Structural lowering:

    • python -m pytest backends/qualcomm/tests/test_passes.py -k "conv_backward_composite_lowering or conv_backward_quantized_inputs_fallback or layout_transform_handles_multiple_tuple_getitems or skip_decomp_table_preserves_conv_backward" -q
    • Passed. Verifies cached HWOI-wrapper reuse, metadata-based rejection of quantized grad_output/weight inputs, unsupported multi-output masks remain in PyTorch layout for fallback, and ConvBackward stays in the QNN skip-decomposition table.
  • HTP rework:

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

    • python -m pytest backends/qualcomm/tests/rework/passes/test.py -k "test_i64_to_i32 and not gpu and not lpai" -q
    • Passed: HTP and HTP-FP16.
    • Covers ConvBackward’s optional tuple-output 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.

❌ 1 Cancelled Job

As of commit 2fa76f4 with merge base 5b14a2f (image):

CANCELLED JOB - The following job was cancelled. Please retry:

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.

This branch has not been deployed

No deployments
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