torch.mm(dense, sparse_csr) #73686
torch.mm(dense, sparse_csr) #73686
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 6ec1929 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
cpuhrsch
force-pushed
the
sparsedensemm1
branch
from
8184274
to
f714a13
Compare
test/test_sparse_csr.py
Outdated
| convert_layout(t) | ||
| convert_layout(m) | ||
| convert_layout(v) |
There was a problem hiding this comment.
These lines do nothing.
There was a problem hiding this comment.
Yes. It's just for intermediate testing. Will remove.
|
@IvanYashchuk - can you take another look? |
| @@ -806,19 +806,23 @@ void spgemm( | |||
| } // anonymous namespace | |||
|
|
|||
| void addmm_out_sparse_csr( | |||
| const at::sparse_csr::SparseCsrTensor& mat1, | |||
| const Tensor& mat1, | |||
There was a problem hiding this comment.
I agree that we should get rid of at::sparse_csr::SparseCsrTensor, this is really good.
|
|
||
| // Returns true if all entries of self are zero | ||
| // TODO: This has potential to be a generic helper | ||
| inline bool _is_all_zero(const Tensor& self) { |
There was a problem hiding this comment.
The strided path of this helper function introduces an unnecessary synchronization. I think it's possible to restructure the code and remove this check. Dense addmm doesn't have checks for zero. nnz == 0 is checked because underlying backend libraries might not handle this case correctly, raising an error, so we need to manually fix the results.
|
|
||
| if (&result != &self) { | ||
| if (result.layout() == kStrided) { | ||
| at::native::resize_output(result, self__sizes); |
There was a problem hiding this comment.
For strided result at::native::resize_output should be used, because there's a deprecation warning:
pytorch/aten/src/ATen/native/Resize.cpp
Lines 17 to 24 in 00607e7
| IntArrayRef self__sizes; | ||
| c10::MaybeOwned<Tensor> self_; | ||
| if (&result != &self && self.layout() == kStrided) { | ||
| self_ = expand_size(self, {mat1_sizes[0], mat2_sizes[1]}, "addmm"); | ||
| self__sizes = self_->sizes(); | ||
| } else { | ||
| self_ = c10::MaybeOwned<Tensor>::borrowed(self); | ||
| self__sizes = self_->sizes(); | ||
| TORCH_CHECK(result.dim() == 2, "tensors must be 2-D"); | ||
| TORCH_CHECK( | ||
| self__sizes[0] == mat1_sizes[0], "self_ dim 0 must match mat1 dim 0"); | ||
| TORCH_CHECK( | ||
| self__sizes[1] == mat2_sizes[1], "self_ dim 1 must match mat2 dim 1"); | ||
| } | ||
| c10::MaybeOwned<at::Tensor> self_ = | ||
| expand_size(self, {mat1.size(0), mat2.size(1)}, "addmm"); |
There was a problem hiding this comment.
This code was copied from addmm_out_cuda_impl so that for in-place operations self is not expanded.
| sparse::impl::_check_is_cpu(self, "self"); | ||
| sparse::impl::_check_is_cpu(mat1, "mat1"); | ||
| sparse::impl::_check_is_cpu(mat2, "mat2"); | ||
| sparse::impl::_check_is_cpu(result, "result"); |
There was a problem hiding this comment.
This might be already in the generated code. The easy way to check this is to pass tensors on different devices and see whether you get an error from these lines or from somewhere higher in the call chain.
There was a problem hiding this comment.
Yes, but then we have to update all the error message tests etc. Sounds like good follow up work.
|
@pytorchbot merge this |
|
Hey @cpuhrsch. |
cpuhrsch
added
release notes: sparse
Summary: Fixes #68621 Pull Request resolved: #73686 Approved by: https://github.com/IvanYashchuk, https://github.com/malfet Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/f2a4d49174eb7b705c2c605c45f78d4fa6786be0 Reviewed By: malfet, b0noI Differential Revision: D34648223 Pulled By: cpuhrsch fbshipit-source-id: 08d62bd1006376d86ee9bb3162848f9f130598b7
Fixes #68621