-
Notifications
You must be signed in to change notification settings - Fork 79
Fix getPointwiseHeuristics with empty tensors #369
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
Closed
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c1761a5
Add C++ and python tests for multiple slices, some empty
jacobhinkle 76c8011
Remove numerator!=0 check.
jacobhinkle 86da47c
Remove split/merge size-0 checks.
jacobhinkle 8f18ffe
Continue if constant zero in multiplyNumeratorValue
jacobhinkle 5c5766b
Merge remote-tracking branch 'origin/main' into projected_extent_size0
jacobhinkle 9b77b77
Use FusionExecutor instead of FEC for easier debugging
jacobhinkle f353c79
Merge remote-tracking branch 'origin/main' into projected_extent_size0
jacobhinkle 5565688
Guard against multiplying by zero extents in ContiguousInnerDimension…
jacobhinkle 14292a7
Merge remote-tracking branch 'origin/main' into projected_extent_size0
jacobhinkle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zasdfgbnm I know you've been looking at this code lately also, in relation to #393 . I am trying to avoid bad behavior when an empty TensorView with size zero extent is encountered. I currently am just pretending it doesn't exist here for the purposes of computing the projected extent. But I'm a little uncertain of whether that is enough, or if it's even safe. I think I have an idea of how this class supposed to work but I'd appreciate you having a look if you have some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should clean up this entire thing with expr simplifier, and I am thinking about doing so after #393.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the case of zero extent, as long as this does not throw an error, whatever value it returns does not matter, because we will not do any memory read/write anyway. Am I correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a tensor has zero elements, should we just stop propagation and assume that tensor is OK with arbitrary vectorization size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am refactoring this part of code for allocation domain support. So I don't want you to invest too much time digging into the codebase on how to do it best. The workaround here makes sense if it can let the test pass.