Skip to content

Conversation

@tarun292
Copy link
Contributor

Summary: Adding support for complex dtypes in op_mul. In this diff i'm only adding support in the optimized path and not in the core portable kernels as that needs support in a bunch of the element-wise macro's that i'm not really familiar with. For now this unblocks our internal use cases.

Differential Revision: D73877440

@pytorch-bot
Copy link

pytorch-bot bot commented Apr 29, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 58b70c0 with merge base 36e44ea (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 Apr 29, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D73877440

@manuelcandales manuelcandales added the release notes: ops & kernels Changes to the opset and any new / changed kernel implementations label Apr 29, 2025
// Disabling this for portable mode for now because currently we only support
// complex dtypes in optimized mode. Portable mode needs support in elementwise_ops.h
// for complex dtypes, which we don't have yet.
//ET_FORALL_COMPLEXH_TYPES(TEST_ENTRY);
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that you are commenting out this line means none of the changes you added in this PR are being tested. I agree you shouldn't be blocked by adding support in the elementwise macros. You have two possible solutions:
a) Add your optimized-only tests under https://github.com/pytorch/executorch/tree/main/kernels/optimized/test
b) Enable complex in portable op_mul without messing around with the elementise macros, just add an if/else and add similar code to the one you added in the optimized op.
I think b) will be way easier

Copy link
Contributor Author

@tarun292 tarun292 Apr 30, 2025

Choose a reason for hiding this comment

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

@manuelcandales took approach b) and updated the PR.

Copy link
Contributor

@manuelcandales manuelcandales left a comment

Choose a reason for hiding this comment

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

Test your code changes either by doing a) or b)

Summary:

Adding support for complex dtypes in op_mul. In this diff i'm only adding support in the optimized path and not in the core portable kernels as that needs support in a bunch of the element-wise macro's that i'm not really familiar with. For now this unblocks our internal use cases.

Differential Revision: D73877440
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D73877440

@facebook-github-bot facebook-github-bot merged commit 2d86424 into main May 1, 2025
85 of 86 checks passed
@facebook-github-bot facebook-github-bot deleted the export-D73877440 branch May 1, 2025 20:01
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. fb-exported release notes: ops & kernels Changes to the opset and any new / changed kernel implementations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants