Qualcomm AI Engine Direct - Support Constant Propogation - #22867
winskuo-quic wants to merge 1 commit into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22867
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit e460e1e with merge base dba8a83 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
4c5c681 to
e460e1e
Compare
|
@claude review this PR |
|
Claude finished @psiddh's task in 2m 56s —— View job Review: Qualcomm AI Engine Direct - Support Constant Propagation
The overall approach makes sense: seed from constant placeholders, propagate in topological order, and only materialize at the boundary. I found two correctness issues and a few design points to look at before merging. I only read the code; I couldn't run the tests here. 🔴 Correctness1. Impure ops (RNG) get folded into a frozen value —
The upstream pass this code was copied from ( if (
node.op != "call_function"
or node.target is memory.alloc
or node.target in all_skip_targets
# Ops with side effects (RNG draws, mutation) have to run at runtime.
or node.is_impure()
):
continuePlease add at least 2. Non-tensor results crash when materialized —
🟡 Design / robustness3.
4. No limit on constant size, and Upstream skips 5. Quantized constant chains through a non-QDQ op aren't tested Rule 3 only protects the case where the boundary node is a dq. Take 6. Pass ordering relative to Both 7. Seeds only come from placeholders —
🟢 Nits
SummaryThe two items I'd fix before merging are #1 (impure/RNG ops get folded) and #2 (a non-tensor result crashes materialization). #3 (the |
Summary
Test plan
pytest backends/qualcomm/tests/rework/passes/test.py -k "test_constant_folding"