Skip to content
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

[MPS] Fix the uint8 type issue with View ops kernels #95145

Closed
wants to merge 1 commit into from

Conversation

razarmehr
Copy link
Collaborator

This should fix the problem in Resnet model with image artifacts due to saturation on int8 type and also the incorrect class recognition reported in #86954.

Fixes #86954

This should fix the problem in Resnet model with image
artifacts due to saturation on int8 type and also
the incorrect class recognition reported in pytorch#86954
@pytorch-bot pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) release notes: mps Release notes category labels Feb 20, 2023
@pytorch-bot
Copy link

pytorch-bot bot commented Feb 20, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 464ff18:
💚 Looks good so far! There are no failures yet. 💚

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

@razarmehr
Copy link
Collaborator Author

@pytorchbot merge -f "Lint and MPS are green"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@razarmehr razarmehr deleted the MPS_view_uint8_fix branch February 20, 2023 18:11
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Feb 22, 2023
This should fix the problem in Resnet model with image artifacts due to saturation on int8 type and also the incorrect class recognition reported in pytorch#86954.

Fixes pytorch#86954

Pull Request resolved: pytorch#95145
Approved by: https://github.com/kulinseth, https://github.com/DenisVieriu97
This was referenced Feb 22, 2023
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Feb 23, 2023
This should fix the problem in Resnet model with image artifacts due to saturation on int8 type and also the incorrect class recognition reported in pytorch#86954.

Fixes pytorch#86954

Pull Request resolved: pytorch#95145
Approved by: https://github.com/kulinseth, https://github.com/DenisVieriu97
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Feb 23, 2023
This should fix the problem in Resnet model with image artifacts due to saturation on int8 type and also the incorrect class recognition reported in pytorch#86954.

Fixes pytorch#86954

Pull Request resolved: pytorch#95145
Approved by: https://github.com/kulinseth, https://github.com/DenisVieriu97
atalman pushed a commit that referenced this pull request Feb 24, 2023
* [MPS] Fix the uint8 type issue with View ops kernels (#95145)

This should fix the problem in Resnet model with image artifacts due to saturation on int8 type and also the incorrect class recognition reported in #86954.

Fixes #86954

Pull Request resolved: #95145
Approved by: https://github.com/kulinseth, https://github.com/DenisVieriu97

* [MPS] Fix tensor with non-zero storage offset graph gathering (#91071)

Previously, the "can slice" flag in Placeholder constructor in `OperationUtils.mm` is conditioned on whether the numbers of dimensions of base shape and view shape are the same. This doesn't consider the situation that a view tensor could be the base tensor's sliced and then unsqueezed version, resulting in different num of dims.

For example, if we want to stack `y_mps` and `x_mps` on the last dim:
```
t_mps = torch.tensor([1, 2, 3, 4], device="mps")
x_mps = t_mps[2:]  # [3, 4]
y_mps = t_mps[:2]  # [1, 2]

res_mps = torch.stack((y_mps, x_mps), dim=-1)
```

the kernel will unsqueeze both of them on the last dim and then concatenate them, which is equivalent to:

```
res_mps = torch.cat((y_mps.unsqueeze(-1), x_mps.unsqueeze(-1)), dim=-1)
```

`x_mps.unsqueeze(-1)` is an unsqueezed and contiguous tensor with a storage offset, this kind of tensors should be sliceable without cloning its storage.

Fixes #87856
Fixes #91065

Pull Request resolved: #91071
Approved by: https://github.com/kulinseth

* [MPS] Fix fill_ where input tensor has a storage offset (#95113)

Fixes #94390

Apart from fixing the issue above, this PR also fixes a bug that when an input tensor can be sliced, a sliced array view is created. This array view seems to be not writable or have a different storage from the original tensor, causing incorrect results with the in-place `fill`.
Pull Request resolved: #95113
Approved by: https://github.com/kulinseth

* [MPS] Fix view op slicing for 2nd dim in case of 0 offset (#95381)

* Fix view op slicing for 2nd dim in case of 0 offset

Pull Request resolved: #95381
Approved by: https://github.com/razarmehr

---------

Co-authored-by: Ramin Azarmehr <razarmehr@apple.com>
Co-authored-by: Li-Huai (Allan) Lin <qqaatw@gmail.com>
Co-authored-by: Denis Vieriu <104024078+DenisVieriu97@users.noreply.github.com>
pruthvistony added a commit to ROCm/pytorch that referenced this pull request May 2, 2023
pruthvistony pushed a commit to ROCm/pytorch that referenced this pull request May 3, 2023
* [MPS] Fix the uint8 type issue with View ops kernels (pytorch#95145)

This should fix the problem in Resnet model with image artifacts due to saturation on int8 type and also the incorrect class recognition reported in pytorch#86954.

Fixes pytorch#86954

Pull Request resolved: pytorch#95145
Approved by: https://github.com/kulinseth, https://github.com/DenisVieriu97

* [MPS] Fix tensor with non-zero storage offset graph gathering (pytorch#91071)

Previously, the "can slice" flag in Placeholder constructor in `OperationUtils.mm` is conditioned on whether the numbers of dimensions of base shape and view shape are the same. This doesn't consider the situation that a view tensor could be the base tensor's sliced and then unsqueezed version, resulting in different num of dims.

For example, if we want to stack `y_mps` and `x_mps` on the last dim:
```
t_mps = torch.tensor([1, 2, 3, 4], device="mps")
x_mps = t_mps[2:]  # [3, 4]
y_mps = t_mps[:2]  # [1, 2]

res_mps = torch.stack((y_mps, x_mps), dim=-1)
```

the kernel will unsqueeze both of them on the last dim and then concatenate them, which is equivalent to:

```
res_mps = torch.cat((y_mps.unsqueeze(-1), x_mps.unsqueeze(-1)), dim=-1)
```

`x_mps.unsqueeze(-1)` is an unsqueezed and contiguous tensor with a storage offset, this kind of tensors should be sliceable without cloning its storage.

Fixes pytorch#87856
Fixes pytorch#91065

Pull Request resolved: pytorch#91071
Approved by: https://github.com/kulinseth

* [MPS] Fix fill_ where input tensor has a storage offset (pytorch#95113)

Fixes pytorch#94390

Apart from fixing the issue above, this PR also fixes a bug that when an input tensor can be sliced, a sliced array view is created. This array view seems to be not writable or have a different storage from the original tensor, causing incorrect results with the in-place `fill`.
Pull Request resolved: pytorch#95113
Approved by: https://github.com/kulinseth

* [MPS] Fix view op slicing for 2nd dim in case of 0 offset (pytorch#95381)

* Fix view op slicing for 2nd dim in case of 0 offset

Pull Request resolved: pytorch#95381
Approved by: https://github.com/razarmehr

---------

Co-authored-by: Ramin Azarmehr <razarmehr@apple.com>
Co-authored-by: Li-Huai (Allan) Lin <qqaatw@gmail.com>
Co-authored-by: Denis Vieriu <104024078+DenisVieriu97@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/mps Run MPS tests (subset of trunk) Merged open source release notes: mps Release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resnet classifier sample returns incorrect results on MPS
5 participants