Skip to content

Revert xnnpack runtime check#18847

Merged
lucylq merged 1 commit intomainfrom
lfq.forward-fix-xnnpack-check
Apr 13, 2026
Merged

Revert xnnpack runtime check#18847
lucylq merged 1 commit intomainfrom
lfq.forward-fix-xnnpack-check

Conversation

@lucylq
Copy link
Copy Markdown
Contributor

@lucylq lucylq commented Apr 13, 2026

Internal failures on: #18799

num_externs <= num_values is not the right check. We should scan xvalues to find num_externs and use that, provided they are valid. Will put up separate PR for the change.

Copilot AI review requested due to automatic review settings April 13, 2026 20:24
@lucylq lucylq requested a review from digantdesai as a code owner April 13, 2026 20:24
@pytorch-bot pytorch-bot bot added the ci-no-td label Apr 13, 2026
@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented Apr 13, 2026

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

⏳ 24 Pending, 1 Unrelated Failure

As of commit 2f2a920 with merge base 651f2f2 (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 13, 2026
@github-actions
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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Reverts a runtime-time validation in the XNNPACK compiler when creating the XNNPACK subgraph, restoring the prior behavior of passing the serialized num_externs directly to xnn_create_subgraph.

Changes:

  • Removed the num_externs <= xvalues->size() guard in XNNCompiler::compileModel.
  • Passed flatbuffer_graph->num_externs() directly as external_value_ids when creating the XNNPACK subgraph.

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

Comment on lines 1862 to 1866
xnn_subgraph_t subgraph_ptr = nullptr;
status = xnn_create_subgraph(
/*external_value_ids=*/num_externs,
/*external_value_ids=*/flatbuffer_graph->num_externs(),
/*flags=*/0,
&subgraph_ptr);
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

Removing the num_externs <= xvalues->size() guard means a malformed/hostile flatbuffer can set num_externs to a very large value, potentially causing excessive allocation/slowdown (or OOM) inside xnn_create_subgraph. Consider adding a safer sanity check instead: e.g., scan xvalues to compute max_external_id actually referenced (and validate external_id < num_externs for any external value), then pass max_external_id + 1 (or a bounded value) to xnn_create_subgraph and return InvalidProgram on inconsistency.

Copilot uses AI. Check for mistakes.
@lucylq lucylq merged commit 3a72c4f into main Apr 13, 2026
168 of 176 checks passed
@lucylq lucylq deleted the lfq.forward-fix-xnnpack-check branch April 13, 2026 21:08
jpiat pushed a commit to jpiat/executorch that referenced this pull request Apr 14, 2026
Internal failures on: pytorch#18799

`num_externs <= num_values` is not the right check. We should scan
xvalues to find `num_externs` and use that, provided they are valid.
Will put up separate PR for the change.

Co-authored-by: Github Executorch <github_executorch@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-no-td CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants