Skip to content

[ET-VK] Clean up api::vTensor class #3149

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
wants to merge 2 commits into from

Conversation

SS-JIA
Copy link
Contributor

@SS-JIA SS-JIA commented Apr 19, 2024

Stack from ghstack (oldest at bottom):

Context

Now that we have forked the api/ directory from PyTorch Vulkan, we can clean up the vTensor class and remove functionality that is not necessary for the ExecuTorch Vulkan delegate.

The following changes are made:

  • Remove unused member variables and member functions from vTensor and vTensorStorage
  • Remove all quantization related member variables, member functions, and the vTensor constructor for quantized tensors. The Quantization API will be reworked from the ground up.
  • Rename view_ (which is an instance of vTensorStorage) to storage_

Finally, the critical change that is introduced is that we now store storage_ as a direct vTensorStorage member variable in vTensor instead of storing it as a std::shared_ptr<vTensorStorage>.

For context, the reason storage_ was stored as a shared pointer is to be compliant with ATen Tensors, which needs to enable copy construction to enable the following:

at::Tensor b = at::rand(...);
// Oftentimes this will create a "view" of the tensor. a and b will point the the same underlying storage, but with different metadata.
at::Tensor a = b;

However, in the ExecuTorch delegate this is no longer necessary. Each Tensor is associated with it's own independent storage and is responsible for managing it's own memory. By getting rid of std::shared_ptr, we can avoid a heap allocation and avoid chasing pointers whenever we need to access the resources of a vTensor.

Differential Revision: D55811279

Copy link

pytorch-bot bot commented Apr 19, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 1863fa6 with merge base 269b6ad (image):
💚 Looks good so far! There are no failures yet. 💚

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

@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 Apr 19, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55811279

## Context

Now that we have forked the `api/` directory from PyTorch Vulkan, we can clean up the `vTensor` class and remove functionality that is not necessary for the ExecuTorch Vulkan delegate.

The following changes are made:

* Remove unused member variables and member functions from `vTensor` and `vTensorStorage`
* Remove all quantization related member variables, member functions, and the `vTensor` constructor for quantized tensors. The Quantization API will be reworked from the ground up.
* Rename `view_` (which is an instance of `vTensorStorage`) to `storage_`

Finally, the critical change that is introduced is that we now store `storage_` as a direct `vTensorStorage` member variable in `vTensor` instead of storing it as a `std::shared_ptr<vTensorStorage>`.

For context, the reason `storage_` was stored as a shared pointer is to be compliant with ATen Tensors, which needs to enable copy construction to enable the following:

```
at::Tensor b = at::rand(...);
// Oftentimes this will create a "view" of the tensor. a and b will point the the same underlying storage, but with different metadata.
at::Tensor a = b;
```

However, in the ExecuTorch delegate this is no longer necessary. Each Tensor is associated with it's own independent storage and is responsible for managing it's own memory. **By getting rid of `std::shared_ptr`, we can avoid a heap allocation and avoid chasing pointers whenever we need to access the resources of a `vTensor`.**

Differential Revision: [D55811279](https://our.internmc.facebook.com/intern/diff/D55811279/)

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55811279

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in bf5093a.

@mergennachin mergennachin mentioned this pull request Apr 26, 2024
@SS-JIA SS-JIA deleted the gh/SS-JIA/40/head branch January 24, 2025 19:39
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. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants