Skip to content

Qualcomm AI Engine Direct - Move QCOM_AXIS_ORDER pop into LayoutTransform pass - #21630

Open
paul1106 wants to merge 1 commit into
pytorch:mainfrom
CodeLinaro:dev1/boru/layoutTransform-refactor
Open

Qualcomm AI Engine Direct - Move QCOM_AXIS_ORDER pop into LayoutTransform pass#21630
paul1106 wants to merge 1 commit into
pytorch:mainfrom
CodeLinaro:dev1/boru/layoutTransform-refactor

Conversation

@paul1106

@paul1106 paul1106 commented Aug 7, 2026

Copy link
Copy Markdown

Summary:

     - The pop of QCOM_AXIS_ORDER was previously done in
     QnnBackend._build_op_wrappers before calling
     transform_for_preprocess_pipeline. Moving it into
     LayoutTransform.call() when insert_permute=True keeps the cleanup
     logic co-located with the code that depends on it.

     The pop is necessary because the delegated subgraph is created via
     deepcopy, which carries over the QCOM_AXIS_ORDER tags written by the
     to-edge LayoutTransform run. Without clearing them, is_transformed_node()
returns True for every sensitive node in the
     main for-loop, causing all traversals to be skipped and no permute
     nodes to be inserted.

Test plan

python -m backends.qualcomm.tests.test_qnn_delegate TestQNNQuantizedOperator  --device ${SERIAL_NUM}   --soc_model ${SOC_MODEL}   --build_folder build-android   --executorch_root .

cc @cccclai @winskuo-quic @shewu-quic @haowhsu-quic @DannyYuyang-quic @cbilgin @abhinaykukkadapu @psiddh

…form pass

Summary:
      - The pop of QCOM_AXIS_ORDER was previously done in
      QnnBackend._build_op_wrappers before calling
      transform_for_preprocess_pipeline. Moving it into
      LayoutTransform.call() when insert_permute=True keeps the cleanup
      logic co-located with the code that depends on it.

      The pop is necessary because the delegated subgraph is created via
      deepcopy, which carries over the QCOM_AXIS_ORDER tags written by the
      to-edge LayoutTransform run. Without clearing them, is_transformed_node()
      returns True for every sensitive node in the
      main for-loop, causing all traversals to be skipped and no permute
      nodes to be inserted.
Copilot AI lite review requested due to automatic review settings August 7, 2026 03:13
@pytorch-bot

pytorch-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 1 Cancelled Job, 1 Unclassified Failure

As of commit 08860de with merge base a2ae4b3 (image):

NEW FAILURE - The following job has failed:

UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:

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 Aug 7, 2026

Copy link
Copy Markdown

Hi @paul1106!

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!

@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 7, 2026

Copy link
Copy Markdown

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

  • ✅ login: paul1106 / name: paul1106 (08860de)

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@paul1106
paul1106 marked this pull request as draft August 7, 2026 03:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a Qualcomm QNN preprocess pipeline issue by relocating cleanup of the QCOM_AXIS_ORDER FX-node metadata tag into the LayoutTransform pass, so the logic that depends on the tag also owns its lifecycle. This prevents a deep-copied delegated subgraph from incorrectly appearing “already transformed,” which previously caused layout-sensitive traversal to be skipped and permute insertion to be missed.

Changes:

  • Remove QCOM_AXIS_ORDER metadata cleanup from QnnBackend._build_op_wrappers.
  • Clear QCOM_AXIS_ORDER inside LayoutTransform.call() when insert_permute is enabled, immediately before the permute-insertion traversal.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
backends/qualcomm/qnn_preprocess.py Removes the pre-pass cleanup of QCOM_AXIS_ORDER from the backend wrapper build step.
backends/qualcomm/_passes/layout_transform.py Adds targeted cleanup of QCOM_AXIS_ORDER within the LayoutTransform permute-insertion flow to avoid skipping sensitive nodes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@paul1106

paul1106 commented Aug 7, 2026

Copy link
Copy Markdown
Author

hi @psiddh could u have a look for my pr please, thanks !
cc: @DannyYuyang-quic

@paul1106

paul1106 commented Aug 7, 2026

Copy link
Copy Markdown
Author

@pytorchbot label "release notes: qualcomm"

@paul1106
paul1106 marked this pull request as ready for review August 7, 2026 06:06
@psiddh

psiddh commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@claude review this pr

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @psiddh's task in 1m 51s —— View job


PR Review: Move QCOM_AXIS_ORDER pop into LayoutTransform pass

  • Read the changed files (layout_transform.py, qnn_preprocess.py)
  • Trace where QCOM_AXIS_ORDER is read/written across passes
  • Verify the new pop placement is behavior-preserving
  • Post review

Verdict: LGTM — this is a correct, well-scoped refactor that also improves the design by co-locating cleanup with the code that depends on it.

Correctness analysis

The move is behavior-preserving, and I verified the key ordering concerns:

  1. QCOM_AXIS_ORDER is only consumed by LayoutTransform among the passes. Grepping backends/qualcomm/_passes/, the only pass that reads the tag is layout_transform.py. The passes that run before LayoutTransform in the preprocess pipeline (FoldQDQ, ConvertMhaToSha, InsertRequantize, InsertIOQDQ — see qnn_pass_manager.py:243-251) never read it. So popping later, inside LayoutTransform.call(), cannot regress any earlier pass. The node builders in qnn_preprocess._build_op_wrappers do read the tag, but they run after the pass re-writes it, so they're unaffected.

  2. The pop is placed at exactly the right point (layout_transform.py:353-359): after the first-run traversal and after transformed_tag is switched back to QCOM_AXIS_ORDER, but before the main sensitive-node loop. The first-run phase uses the QCOM_LAYOUT_CHANGE tag for is_transformed_node(), so the stale QCOM_AXIS_ORDER tags carried over by the deepcopy don't interfere with it. Popping only right before the main loop is what makes the sensitive nodes eligible for traversal again — exactly the bug described in the summary.

  3. The to-edge run is unchanged. That run uses insert_permute=False (the constructor default), so the new pop block — guarded by if self.insert_permute: — is skipped there. The tag is only cleared in the preprocess path where insert_permute=True, which is always the delegated deepcopy. No legitimate tags are discarded.

Minor observations (non-blocking)

  • node.meta.pop(QCOM_AXIS_ORDER, "") — the "" default is never consumed. It's carried over verbatim from the old code, so no need to change, but None would read more conventionally as an "ignore if missing" sentinel.
  • hasattr(node, "meta") — FX nodes always have .meta, so the guard is effectively always true. Also preserved from the original code; fine to leave for parity.

Testing

The PR is covered by an on-device test (TestQNNQuantizedOperator), which is the right suite since the change affects permute insertion in the delegated preprocess path. CI workflows are currently awaiting approval on this PR, so results aren't visible yet.
· branch dev1/boru/layoutTransform-refactor

@nil-is-all nil-is-all added partner: qualcomm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Qualcomm module: qnn Issues related to Qualcomm's QNN delegate and code under backends/qualcomm/ labels Aug 10, 2026
@psiddh

psiddh commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@paul1106 can you address this pls ? Meta CLA Check — Missing Contributor License Agreement!

@qti-horodnic qti-horodnic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Minor: With this change, the following code block is now dead: https://github.com/pytorch/executorch/blob/main/backends/qualcomm/tests/rework/passes/passes_helper.py#L290-L295

Can you also remove it as part of this PR and run test_layout_transform in the rework framework to validate?

@psiddh

psiddh commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

I you may want to rebase your PR to get past the Lint / link-check / lint-urls (pull_request)

@shewu-quic shewu-quic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks

@winskuo-quic winskuo-quic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thanks for moving this inside the LayoutTransform to improve readability.

@psiddh

psiddh commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@paul1106 can you complete Meta CLA Check ?

@DannyYuyang-quic

Copy link
Copy Markdown
Contributor

@psiddh Hi, sorry I forgot to inform you. @paul1106 is our intern and will be helping us on related tasks.
He's out of office this week and will complete the Meta CLA check when he's back next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: qnn Issues related to Qualcomm's QNN delegate and code under backends/qualcomm/ partner: qualcomm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Qualcomm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants