Skip to content

Conversation

@jenniew
Copy link

@jenniew jenniew commented Nov 26, 2025

Sparse ops on SparseXPU:
sparse addmm

Related issue: #2211

@jenniew jenniew requested a review from CuiYifeng November 26, 2025 01:35
Copy link
Contributor

@CuiYifeng CuiYifeng left a comment

Choose a reason for hiding this comment

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

Main part of this PR LGTM.

TORCH_CHECK(sparse_.sparse_dim() == 2, "addmm: expected first two dims to be sparse (indices has size 2 at first dim), but got ", sparse_.sparse_dim(), " sparse dims");
// no need to check dense_dim because dense_dim + sparse_dim = dim

Tensor mat1_dense = sparse_._to_dense(std::nullopt, std::nullopt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will Tensor mat1_dense = sparse_._to_dense() cause undefined issues? I noticed that dtype and masked are optional for sparse_to_dense.

TORCH_CHECK(sparse_.is_xpu(), "Expected all tensors to be on the same device. addmm: expected 'mat1' to be XPU, but got CPU");
TORCH_CHECK(dense.is_xpu(), "Expected all tensors to be on the same device. addmm: expected 'mat2' to be XPU, but got CPU");

// TORCH_CHECK(xpu::check_device({sparse_, r_, t, dense}));
Copy link
Contributor

Choose a reason for hiding this comment

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

xpu::check_device can be found in sparse/xpu/sycl/SparseTensorMathKernels.cpp.

@CuiYifeng CuiYifeng requested a review from Copilot November 26, 2025 03:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements sparse addmm operations for XPU devices by adding support for addmm operations on sparse tensors. The implementation follows the existing PyTorch sparse tensor patterns by converting sparse tensors to dense format and delegating to the standard addmm operations.

Key changes:

  • Added three addmm function variants (standard, out-variant, and in-place) to the native functions YAML with SparseXPU dispatch keys
  • Implemented the sparse addmm operations in C++ by converting sparse tensors to dense format
  • Removed the test skip decorator to enable existing sparse addmm tests for XPU

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
yaml/native/native_functions.yaml Registered three addmm function variants with SparseXPU dispatch mappings
test/xpu/test_sparse_xpu.py Removed skipXPUIf decorator to enable sparse addmm tests
src/ATen/native/sparse/xpu/SparseTensorMath.cpp Implemented sparse addmm functions with device validation and sparse-to-dense conversion
Comments suppressed due to low confidence (1)

yaml/native/native_functions.yaml:1

  • Corrected spelling of 'sprase' to 'sparse' in the skip message.
- func: copy(Tensor self, Tensor src, bool non_blocking=False) -> Tensor

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

TORCH_CHECK(sparse_.is_xpu(), "Expected all tensors to be on the same device. addmm: expected 'mat1' to be XPU, but got CPU");
TORCH_CHECK(dense.is_xpu(), "Expected all tensors to be on the same device. addmm: expected 'mat2' to be XPU, but got CPU");

// TORCH_CHECK(xpu::check_device({sparse_, r_, t, dense}));
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

This commented-out device check should either be implemented or removed. If device validation is needed beyond the individual tensor checks above, uncomment and ensure the function exists; otherwise, remove the dead code.

Suggested change
// TORCH_CHECK(xpu::check_device({sparse_, r_, t, dense}));

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +51
Tensor mat1_dense = sparse_._to_dense(std::nullopt, std::nullopt);
at::addmm_out(r_, t, mat1_dense, dense, beta, alpha);
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

Converting the sparse tensor to dense format defeats the purpose of sparse operations and may cause significant memory overhead for large sparse tensors. Consider implementing a proper sparse matrix multiplication kernel instead of this fallback approach.

Suggested change
Tensor mat1_dense = sparse_._to_dense(std::nullopt, std::nullopt);
at::addmm_out(r_, t, mat1_dense, dense, beta, alpha);
// Use a proper sparse matrix multiplication kernel for XPU
xpu::addmm_out_sparse_dense_kernel(r_, t, sparse_, dense, beta, alpha);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants