Skip to content

Conversation

@abhinaykukkadapu
Copy link
Contributor

@abhinaykukkadapu abhinaykukkadapu commented Jun 14, 2025

Summary

This PR uses xnn_define_binary and xnn_define_unary to define XNNPack ops, instead of separately calling the individual definitions.

Further changes:

  1. Removes individual node definitions for unary and binary ops
  2. Creates a wrapper macro to generate function defs for individual ops using xnn_define_binary and xnn_define_unary inside.

Fixes #11584

Test plan

## Build steps
cmake -DEXECUTORCH_BUILD_XNNPACK=ON ..
cmake --build cmake-out -j9

Tests ran:
./test/run_oss_cpp_tests.sh
.
.
.
100% tests passed, 0 tests failed out of 86

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 14, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit f39f3ee with merge base 7b39a0c (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-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 Jun 14, 2025
@abhinaykukkadapu
Copy link
Contributor Author

@pytorchbot label "release notes: none"

@pytorch-bot pytorch-bot bot added the release notes: none Do not include this in the release notes label Jun 14, 2025
@abhinaykukkadapu abhinaykukkadapu marked this pull request as ready for review June 14, 2025 16:48
@abhinaykukkadapu
Copy link
Contributor Author

lint runner failure seems unrelated. Local lint run is successful:

$ lintrunner -a
Warning: Could not find a lintrunner config at: '.lintrunner.private.toml'. Continuing without using configuration file.
  FLAKE8 success!
  CMAKE success!
  CLANGFORMAT success!
  UFMT success!
  ETCAPITAL success!
  NEWLINE success!
  NOTORCHINC success!
  NOSTDINC success!
  MYPY success!
  LICENSELINT success!
  TORCH_AO_IMPORT success!
ok No lint issues.
Successfully applied all patches.

@abhinaykukkadapu abhinaykukkadapu force-pushed the use_xnnpack_defines_unary_binary branch 2 times, most recently from 92a98f4 to 464da84 Compare June 16, 2025 17:10
@mcr229
Copy link
Contributor

mcr229 commented Jun 16, 2025

Seems like a transient issue with lint runner for now. Thanks for taking this up. In XNNCompiler we have large switch statements that look like this:

DefineNodeFunc getDefineNodeFunc(fb_xnnpack::XNodeUnion nodeType) {

since the define_binary/unary all look like pretty much the same implementation, would it be possible to refactor some of these _DEFINE into _DEFINE_UNARY, _DEFINE_BINARY So that we don't need custom defineSubNode, defineAddNode. I know for a couple unary configs there is some variation with alpha so there is custom logic there, but this would defintely help a lot for readability.

@abhinaykukkadapu
Copy link
Contributor Author

@mcr229 please let me know if you suggest any more tests.

@abhinaykukkadapu abhinaykukkadapu merged commit 0286927 into pytorch:main Jun 17, 2025
96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: none Do not include this in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move to binary/unary subgraph

3 participants