-
Notifications
You must be signed in to change notification settings - Fork 157
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
eStorage tensor fix, eval clears operations in sequence, added one test and modified tests that were broken #304
Closed
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
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
…s recorded to sequence Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Signed-off-by: Miro Palmu <miro.palmu@helsinki.fi>
Closed
BTW I did not figure out hot to generate doctrings.hpp automatically so I modified it manually. |
Closing as superceeded by #316 |
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.
This might might be too large PR. Sorry about that. Because of my mistake PR #302 is also included in this.
Two main things are that we can now create eStorage tensors with nullptr as data without undefined behaviour and evalAwait (and functions calling it) now clear operations stored in sequence.
Added a concept "device only" tensors that can be checked with isDeviceOnlyTensor(). At the moment eStorage is only device only tensor.
This is used to determine if we need to do raw data mappings and unmappings. Also in postEval of OpTensorCopy made it such that it does not copy raw data if the tensor that we copy from is device only nor it does not copy raw data to a tensor that is device only.
This fixes undefined behaviour that we had with eStorage tensor. Now we can create device only tensors with manager by setting data to nullptr.
I added one test about copying in and out from eStorage tensors.
Motivation behind changes: There was lot of tests that were chaining evals together. For example
This is straight up broken because we never clear vk::CommandBuffer nor operations stored in the sequence. When second eval is called it runs OpTensorSyncDevice again because it never got cleared form the sequence. This can be checked from the logs.
Because I think chaining evals is quite intuitive api and I like it I wanted to fix this so what I propose is that evalAwait would always clear the operations stored in the sequence. So now you have to manually record operations again but I don't think this is a problem.
I added vk::CommandBufferUsageFlagBits::eOneTimeSubmit to command buffer create info such that we are sure vk::CommandBuffer gets cleared every time we start record to it.
With this change some tests got broken or they had bugs in them. I also fixed them.