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.

Sign up for GitHub

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

Jump to bottom

Introduce fast path in the CPU equal op #100024

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Introduce fast path in the CPU equal op #100024

merged 1 commit into from
Apr 28, 2023

Conversation

houseroad
Copy link
Member

Summary: When two tensors share the same storage, and strides, and no other flags, then we should consider this tensors as equal.

Test Plan: buck2 test @//mode/opt //caffe2/test:torch -- --exact 'caffe2/test:torch - test_equal (test_torch.TestTorch)'

Differential Revision: D45282119

@pytorch-bot
Copy link

pytorch-bot bot commented Apr 25, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 3e92a89:
💚 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
Copy link
Contributor

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

albanD
albanD reviewed Apr 25, 2023
View reviewed changes
aten/src/ATen/native/ReduceOps.cpp Outdated
// ensuring the storage and strides exactly the same.
if (self.sizes().equals(other.sizes())
&& self.strides().equals(other.strides())
&& self.storage().is_alias_of(other.storage())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since storage is untyped, you should check the the dtype() matches here as well.
A good test for this would be:

import torch

a = torch.rand((2, 2), dtype=torch.float)
b = a.view(dtype=torch.int32)

print(torch.equal(a, b))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Added

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

ezyang
ezyang reviewed Apr 26, 2023
View reviewed changes
aten/src/ATen/native/ReduceOps.cpp
&& self.storage_offset() == other.storage_offset()
&& self.layout() == other.layout()
&& self.is_neg() == other.is_neg()
&& self.is_conj() == other.is_conj()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to test these three, they will have been handled before getting here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be safe, I am a bit concerned in some cases, if users directly this cpu_equal function directly, although this should be rare. Keeping these checks shouldn't hurt, or we concern about the overhead for these calls?

ezyang
ezyang reviewed Apr 26, 2023
View reviewed changes
aten/src/ATen/native/ReduceOps.cpp Outdated
// TensorIterator, it should be safe to have the following fast path by
// ensuring the storage and strides exactly the same.
if (self.dtype() == other.dtype()
&& self.sizes().equals(other.sizes())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to test this, it's tested above

ezyang
ezyang reviewed Apr 26, 2023
View reviewed changes
aten/src/ATen/native/ReduceOps.cpp Outdated
// ensuring the storage and strides exactly the same.
if (self.dtype() == other.dtype()
&& self.sizes().equals(other.sizes())
&& self.strides().equals(other.strides())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fastpath for this would be to instead assert both tensors are contiguous, before checking their strides

ezyang
ezyang approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also want this to apply to cuda too.

@houseroad
Copy link
Member Author

Yeah, I will handle the CUDA one in the following PR.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

@houseroad @facebook-github-bot
Introduce fast path in the CPU equal op (pytorch#100024)
3e92a89 
Summary:
Pull Request resolved: pytorch#100024

When two tensors share the same storage, and strides, and no other flags, then we should consider this tensors as equal.

We have another approach in pytorch#99703, which is directly check equality in the JIT loader. However, we may have to handle the flags like neg/conj explicitly. It's a bit hard to cover all the cases. Per discussion with davidberard98, in the flags like neg/conj should be handled by the dispatcher already (and the TensorIterator logic also proves this), so adding the fast path to CPU and CUDA ops should be a better/safer approach.

Test Plan: buck2 test @//mode/opt //caffe2/test:torch -- --exact 'caffe2/test:torch - test_equal (test_torch.TestTorch)'

Reviewed By: hyuen

Differential Revision: D45282119

fbshipit-source-id: 18e939d236a6d84a79013317db8b2f715f4a3cff
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot merged commit d7fa7fa into pytorch:main Apr 28, 2023
@huydhn huydhn mentioned this pull request Apr 30, 2023
Closed
This was referenced Oct 14, 2023
Closed
Closed
malfet added a commit that referenced this pull request Oct 20, 2023
@malfet
Fix regression in torch.equal behavior for NaNs
cfb3bf8 
`torch.equal(x, x)` should return false if one of `x` is a tenor of
floating point values one of which could be NaN
So, it renders some of the optimization proposed in
#100024 invalid.

Add regression test that calls torch.equal for tensor containing NaN

Fixes #111251
pytorchmergebot pushed a commit that referenced this pull request Oct 21, 2023
@malfet @pytorchmergebot
Fix regression in torch.equal behavior for NaNs (#111699)
7709382 
`torch.equal(x, x)` should return false if one of `x` is a tenor of floats one of which is NaN.
So, it renders some of the optimization proposed in #100024 invalid, though as result `torch.equal` will become much slower for identical floating point tensors.

Add regression test that calls torch.equal for tensor containing NaN

Fixes #111251

Pull Request resolved: #111699
Approved by: https://github.com/Skylion007, https://github.com/albanD
@malfet malfet mentioned this pull request Oct 25, 2023
Merged
malfet added a commit that referenced this pull request Oct 25, 2023
@malfet
Fix regression in torch.equal behavior for NaNs (#111699)
3eaa411 
`torch.equal(x, x)` should return false if one of `x` is a tenor of floats one of which is NaN.
So, it renders some of the optimization proposed in #100024 invalid, though as result `torch.equal` will become much slower for identical floating point tensors.

Add regression test that calls torch.equal for tensor containing NaN

Fixes #111251

Pull Request resolved: #111699
Approved by: https://github.com/Skylion007, https://github.com/albanD

(cherry picked from commit 7709382)
atalman pushed a commit that referenced this pull request Oct 25, 2023
@malfet
Fix regression in torch.equal behavior for NaNs (#111699) (#111996)
736ebd3 
`torch.equal(x, x)` should return false if one of `x` is a tenor of floats one of which is NaN.
So, it renders some of the optimization proposed in #100024 invalid, though as result `torch.equal` will become much slower for identical floating point tensors.

Add regression test that calls torch.equal for tensor containing NaN

Fixes #111251

Pull Request resolved: #111699
Approved by: https://github.com/Skylion007, https://github.com/albanD

(cherry picked from commit 7709382)
xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023
@malfet @xuhancn
Fix regression in torch.equal behavior for NaNs (pytorch#111699)
e8e4da2 
`torch.equal(x, x)` should return false if one of `x` is a tenor of floats one of which is NaN.
So, it renders some of the optimization proposed in pytorch#100024 invalid, though as result `torch.equal` will become much slower for identical floating point tensors.

Add regression test that calls torch.equal for tensor containing NaN

Fixes pytorch#111251

Pull Request resolved: pytorch#111699
Approved by: https://github.com/Skylion007, https://github.com/albanD
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
@malfet @Skylion007
Fix regression in torch.equal behavior for NaNs (pytorch#111699)
1935939 
`torch.equal(x, x)` should return false if one of `x` is a tenor of floats one of which is NaN.
So, it renders some of the optimization proposed in pytorch#100024 invalid, though as result `torch.equal` will become much slower for identical floating point tensors.

Add regression test that calls torch.equal for tensor containing NaN

Fixes pytorch#111251

Pull Request resolved: pytorch#111699
Approved by: https://github.com/Skylion007, https://github.com/albanD
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albanD albanD albanD left review comments

@ezyang ezyang ezyang approved these changes

@davidberard98 davidberard98 Awaiting requested review from davidberard98

Assignees
No one assigned
Labels
fb-exported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@houseroad @facebook-github-bot @ezyang @albanD