Skip to content

Add optimized op_where #8866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 28 commits into from
Mar 6, 2025
Merged

Add optimized op_where #8866

merged 28 commits into from
Mar 6, 2025

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Mar 1, 2025

It materializes separate kernels for the cases where the two input data tensors have the same dtype and the third one has dtype bool.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@swolchok
Copy link
Contributor Author

swolchok commented Mar 1, 2025

Copy link

pytorch-bot bot commented Mar 1, 2025

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit e52508d with merge base cca6917 (image):

NEW FAILURE - The following job has failed:

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

swolchok added a commit that referenced this pull request Mar 1, 2025
ghstack-comment-id: 2691805026

ghstack-source-id: ebe8bee
ghstack-comment-id: 2691808920
Pull Request resolved: #8866
@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 Mar 1, 2025
swolchok added 4 commits March 3, 2025 10:03
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 3, 2025
ghstack-comment-id: 2691805026

ghstack-source-id: aaf6617
ghstack-comment-id: 2691808920
Pull Request resolved: #8866
[ghstack-poisoned]
swolchok added 2 commits March 3, 2025 15:31
[ghstack-poisoned]
[ghstack-poisoned]
Comment on lines +63 to +64
data_out[out_index] =
data_cond[cond_index] ? data_a[a_index] : data_b[b_index];
Copy link
Contributor

@digantdesai digantdesai Mar 4, 2025

Choose a reason for hiding this comment

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

Love how clean it reads :)

Couple of high level comments,

  • Not too familiar with how people use this op i.e. any common case we see often in the wild, but are there any short-circuits we can do to avoid coming here?
  • Similarly, depending on the condition, if we know we are biased towards A vs. B, we can first copy A to the result and then go through data_cond and pick B.
  • Lastly, I am assuming we want to do SIMD later, if we do then we can use predicates, that should make it less "branchy". It may not help much with load/stores though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we know we are biased towards A vs. B

we can't possibly know this in general.

SIMD later

not currently on my agenda, but I may have to come back.

Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

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

Looks good to me.

swolchok added 2 commits March 4, 2025 10:27
[ghstack-poisoned]
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 4, 2025
ghstack-comment-id: 2691805026

ghstack-source-id: 7e5ac3a
ghstack-comment-id: 2691808920
Pull Request resolved: #8866
swolchok added 3 commits March 4, 2025 15:35
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 4, 2025
ghstack-comment-id: 2691805026

ghstack-source-id: c88f938
ghstack-comment-id: 2691808920
Pull Request resolved: #8866
swolchok added 2 commits March 4, 2025 21:32
[ghstack-poisoned]
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 5, 2025
ghstack-comment-id: 2691805026

ghstack-source-id: 1a0f6d2
ghstack-comment-id: 2691808920
Pull Request resolved: #8866
swolchok added 2 commits March 5, 2025 09:50
[ghstack-poisoned]
[ghstack-poisoned]
swolchok added 4 commits March 5, 2025 10:04
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
Base automatically changed from gh/swolchok/301/head to main March 6, 2025 03:06
@swolchok swolchok changed the title first crack at optimized op_where Add optimized op_where Mar 6, 2025
@swolchok
Copy link
Contributor Author

swolchok commented Mar 6, 2025

unittest-editable failure is a known flake, so noting that we have green CI. I think this will cause -Wunused -Werror builds to fail though, so I need to fix that before merging.

@swolchok
Copy link
Contributor Author

swolchok commented Mar 6, 2025

I think this will cause -Wunused -Werror builds to fail though

Checked, works fine, I misread my own code.

@swolchok swolchok merged commit a2c0b59 into main Mar 6, 2025
50 of 51 checks passed
@swolchok swolchok deleted the gh/swolchok/302/head branch March 6, 2025 18:37
zonglinpeng pushed a commit that referenced this pull request Mar 6, 2025
It materializes separate kernels for the cases where the two input data tensors have the same dtype and the third one has dtype bool.
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: 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