-
Notifications
You must be signed in to change notification settings - Fork 79
[Host irs] alias and preallocated output support #4144
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
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
9a0dc9e
host ir alias and prealloc output support
samnordmann 9820d5a
harden and simplify allocation in for loop test
samnordmann 8c49c95
Merge branch 'main' of github.com:NVIDIA/Fuser into host_irs/alias_su…
samnordmann e1db518
reviews
samnordmann 7e6cef6
Merge branch 'main' of github.com:NVIDIA/Fuser into host_irs/alias_su…
samnordmann eb46aef
minor comment
samnordmann 5f161f5
lint
samnordmann 97b1743
Merge branch 'main' of github.com:NVIDIA/Fuser into host_irs/alias_su…
samnordmann 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
Oops, something went wrong.
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.
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.
Why not TensorView* since markAlias takes TensorView?
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.
It is more convenient imo to keep a
Val*here to have a uniform treatment of Scalars and TensorViews when we get/set, through the methodgetAlias.If changing the signature to
TensorViewhere, we need to branch intogetAliasto maybe downcast theVal*to aTensorView*, get the alias, and then upcast back the obtainedTensorView*to aVal*.This is doable but imo slightly less natural.
Since, besides, we restrict aliasing to TensorView only for simplicity, but not for a structural reason, this is why I made this choice.
Let me know what you prefer.
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.
What does it mean for a scalar to alias another scalar?
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.
Like for a Tensor: a different name pointing to the same data, (IOW seeing the scalar as a 0-dim tensor)
But there is no real motivation to support that for now.
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.
FYI, scalar is not a 0-dim tensor. The difference is like
intvsint*. To me, aliases for pointers make sense but aliases for scalars don't.I'm fine with this code as is. Personally, I prefer
std::unordered_map<TensorView*, TensorView*>because it gives the right impression that only TensorViews can alias.std::unordered_map<Val*, TensorView*>is OK as well if the key is too often aVal*and you want to save too much typing of->as<TensorView>().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.
Ok I leave it as is for now. For the sake of the discussion though and to make sure I am not missing something:
A scalar can be viewed as a 0-dim tensor, mathematically. Then, it is an implementation detail whether the scalar type owns a pointer or a value. I understand that pytorch makes the choice that
at::Scalarrepresents the value and not the pointer, contrarily toat::Tensor. However, in our context, we are adding one level of indirection (through hir-aliasing and expression evaluator), so aliasing is always possible, even for scalars. More precisely: the symbolic object (scalar or tensor) is aVal*, mapped through (hir-alias+ExprEvaluator) to aPolymorphicValue(e.g. aat::Tensoror aat::Scalar). TwoVal*can always be mapped to the same concrete value, IOW, aliasing is always possible.