Qualcomm AI Engine Direct - Add HTP context graph splitting option - #22595
chenweng-quic wants to merge 3 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22595
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit ccb9f02 with merge base 5e21c13 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
0fa0ae7 to
aed61f4
Compare
|
Hi @psiddh, |
|
@claude thoroughly review this PR , also review it from any potential regression stand point |
|
Claude finished @psiddh's task in 3m 30s —— View job PR Review: HTP context graph-splitting optionReviewed the full diff against the QNN backend. Overall this is a clean, well-scoped, opt-in feature with the right default ( 🔴 Potential bug — uninitialized struct fields
QnnHtpContext_GraphSplit_t graph_split_info; // not initialized
graph_split_info.graphSplittingEnabled = true;
p_custom_config->graphSplittingConfigs = graph_split_info;Only QnnHtpContext_GraphSplit_t graph_split_info = QNN_HTP_CONTEXT_GRAPH_SPLIT_INIT; // if the SDK defines it
// or, at minimum:
QnnHtpContext_GraphSplit_t graph_split_info{}; // zero-init all fields🟠 Version-gate logic compares major/minor independently
#if (QNN_HTP_API_VERSION_MAJOR >= 5 && QNN_HTP_API_VERSION_MINOR >= 49)This does not correctly express "HTP API ≥ 5.49". A future #if (QNN_HTP_API_VERSION_MAJOR > 5 || (QNN_HTP_API_VERSION_MAJOR == 5 && QNN_HTP_API_VERSION_MINOR >= 49))(The existing 🟠 Warning message prints the wrong version macros
🟡 Tests leak an artifact on failure
self.assertTrue(os.path.isfile("forward_schematic.bin_sg_0.py"))
os.remove("forward_schematic.bin_sg_0.py")If the self.addCleanup(lambda: os.path.exists("forward_schematic.bin_sg_0.py") and os.remove("forward_schematic.bin_sg_0.py"))
self.assertTrue(os.path.isfile("forward_schematic.bin_sg_0.py"))🟡 Version-scheme consistency between Python and C++The Python tests gate on ✅ Regression assessment
Minor
Nice, focused PR — addressing the struct init (#1) is the one I'd consider blocking; the rest are polish. |
2c6b578 to
1606852
Compare
86835b8 to
99abe96
Compare
Exposes QNN_HTP_CONTEXT_CONFIG_OPTION_GRAPH_SPLITTING_CONFIGS through generate_htp_compiler_spec(use_graph_splitting=...) for the offline-prepare (host) export path. Gated behind QNN HTP API >= 5.49 since this option does not exist in older version. Co-authored-by with assistance from Claude Code (Anthropic).
99abe96 to
ccb9f02
Compare
|
do you know what this failure is ? MLTEC_elastic - GitHubPOC - executorch dev1-chenweng-graph_split |
| # file for subgraph 0 | ||
| # delete artifact before assertion to avoid leak | ||
| file_name = "forward_schematic.bin_sg_0.py" | ||
| file_exist = os.path.isfile(file_name) |
There was a problem hiding this comment.
os.path.isfile accepts a file descriptor, so this stats fd 1 or 0 - stdout/stdin not the artifact, did I miss anything ?
There was a problem hiding this comment.
it accepts a file path.
Hi @psiddh , |
Summary
QNN_HTP_CONTEXT_CONFIG_OPTION_GRAPH_SPLITTING_CONFIGS/QnnHtpContext_GraphSplit_t) via a newuse_graph_splittingoption ongenerate_htp_compiler_spec(), see https://docs.qualcomm.com/doc/80-63442-10/topic/htp_backend.html. This option significantly reduces finalization time at the cost of a slight increase in execution time.Misc
QNN_HTP_API_VERSION_MAJOR >= 5 && QNN_HTP_API_VERSION_MINOR >= 49since this option doesn't exist in older QNN_HTP_API_VERSIONTest plan
LLAMA3.2 3B performance:
This table is created by extracting data from finalization log:

This PR was authored with assistance from Claude Code (Anthropic).
cc @cbilgin @psiddh