[functorch] Prevent using for-loop for out-of-place index_fill batch rule #99229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rule [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99229
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit b77fce9: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
qqaatw
added a commit
that referenced
this pull request
Apr 15, 2023
…rule ghstack-source-id: 2d2ba335785096dd17779c48fa8b991624739822 Pull Request resolved: #99229
qqaatw
added
the
release notes: functorch
…fill batch rule" A follow-up PR for #91364 (comment) [ghstack-poisoned]
qqaatw
added a commit
that referenced
this pull request
Apr 17, 2023
…rule ghstack-source-id: f31f7166b41d8dca2c6b3bdb94bba4478600ad2b Pull Request resolved: #99229
…fill batch rule" A follow-up PR for #91364 (comment) [ghstack-poisoned]
qqaatw
added a commit
that referenced
this pull request
Apr 17, 2023
…rule ghstack-source-id: d7ebf37f89f9117de99198475794f78963aa93ce Pull Request resolved: #99229
| self_slice.index_fill_( | ||
| dim, | ||
| index_slice, | ||
| value_bdim.has_value() ? value_.select(0, i) : value_ |
There was a problem hiding this comment.
Can we write this for-loop such that we won't have to check value_bdim every iteration?
There was a problem hiding this comment.
I feel this is the most concise version, and checking value_bdim would not be costly.
Here are the ways I came up:
- Always ensure
valuehas a batch dim and do select in every iteration - costly. - Make copies of the for-loop statement and separate the work out. There are four combinations for
inplaceandvalue_bdim.has_value()- not concise. - Determine
value_bdim.has_value()before for-loop and use lambdas to wrap eithervalue_orvalue_.select(0, i)- I feel unnecessary tbh. Would it be better?
Or maybe I'm missing something.
There was a problem hiding this comment.
Hmm, makes sense, plus I would assume that most of the time would be spent in index_fill so this should be good.
…fill batch rule" A follow-up PR for #91364 (comment) cc zou3519 Chillee samdow soumith kshitij12345 janeyx99 [ghstack-poisoned]
qqaatw
added a commit
that referenced
this pull request
Apr 18, 2023
…rule ghstack-source-id: b5feebe0e0d028dac696482ef77c2a5dcc00a6c1 Pull Request resolved: #99229
…fill batch rule" A follow-up PR for #91364 (comment) cc zou3519 Chillee samdow soumith kshitij12345 janeyx99 [ghstack-poisoned]
qqaatw
added a commit
that referenced
this pull request
Apr 18, 2023
…rule ghstack-source-id: ad2b76877d4d788d9e5b77bc8c2e40c853d6a331 Pull Request resolved: #99229
…fill batch rule" A follow-up PR for #91364 (comment) cc zou3519 Chillee samdow soumith kshitij12345 janeyx99 [ghstack-poisoned]
qqaatw
added a commit
that referenced
this pull request
Apr 19, 2023
…rule ghstack-source-id: 60b5301fc7ae924142a16b044b21a2b90dd1cf24 Pull Request resolved: #99229
kshitij12345
approved these changes
Apr 20, 2023
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
A follow-up PR for #91364 (comment)
cc @zou3519 @Chillee @samdow @soumith @kshitij12345 @janeyx99