Skip to content

Conversation

@Aidyn-A
Copy link
Collaborator

@Aidyn-A Aidyn-A commented May 18, 2023

This pull request adds a functionality for addmm with the following layouts:

    add(a_csr, b_csr)

    addmm(c_csr, a_csr, b_csr)
    addmm(c_csr, a_csr, b_csc)
    addmm(c_csr, a_csc, b_csc)
    addmm(c_csr, a_csc, b_csr)

using Eigen for non-MKL builds.

Edit: thanks to the comment #101814 (comment) the following ops were also added:

    add(a_csc, b_csc)

    addmm(c_csc, a_csr, b_csr)
    addmm(c_csc, a_csr, b_csc)
    addmm(c_csc, a_csc, b_csc)
    addmm(c_csc, a_csc, b_csr)

@pytorch-bot
Copy link

pytorch-bot bot commented May 18, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit e92166b with merge base e872bf8 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the release notes: sparse release notes category label May 18, 2023
@Aidyn-A Aidyn-A marked this pull request as ready for review May 19, 2023 01:23
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented May 19, 2023

@pytorchbot label "ciflow/trunk"

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 19, 2023
@jbschlosser jbschlosser requested review from amjames, cpuhrsch and pearu and removed request for amjames May 19, 2023 20:52
@jbschlosser jbschlosser added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 19, 2023
@cpuhrsch
Copy link
Contributor

Thank you for the pull request @Aidyn-A - which issue does this address? Do you have timings on the performance?

@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented May 23, 2023

Hi @cpuhrsch,
The reason why this PR exist is that sparse operations add, addmm and matmul are not available on CPUs without MKL. Therefore, the following architectures are left out:

  • All machines with ARM based CPUs
  • All Windows OS machines
  • All modern Mac machines

Among existing issues I found this one:

I do not have performance numbers at the moment, but I can provide some.

Copy link
Collaborator

@amjames amjames left a comment

Choose a reason for hiding this comment

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

Couple of comments, however all of them assume that there is general agreement on introducing an eigen dependency.

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cpuhrsch
Copy link
Contributor

I'm going to import this to get a sense for the internal build setup and whether this needs to be co-landed.

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cpuhrsch
Copy link
Contributor

Various internal build system changes should make this GH1 landable, but I'm reimporting to verify. I'll be on PTO in a few days, but back July 10th. If it doesn't land before I go on PTO, I'll pick this back up then.

@@ -0,0 +1,381 @@
#include <ATen/native/eigen/SparseBlasImpl.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this file into
aten/src/ATen/native/*sparse*/eigen/SparseBlasImpl.cpp instead of aten/src/ATen/native/eigen/SparseBlasImpl.cpp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hope 6a91e00 is what you meant

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Aug 9, 2023

Hi @cpuhrsch,
Hope you were able to apply the system changes you mentioned. Shall I rebase this PR to be up-to-date with the latest PyTorch?

@cpuhrsch
Copy link
Contributor

@Aidyn-A - Yes, could you please try to rebase and I'll confirm that it works internally? Please mention me once that's done.

@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Sep 12, 2023

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased eigen_for_sparse_addmm onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout eigen_for_sparse_addmm && git pull --rebase)

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased eigen_for_sparse_addmm onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout eigen_for_sparse_addmm && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the eigen_for_sparse_addmm branch from 5d21600 to cec2a79 Compare March 11, 2025 16:38
@github-actions github-actions bot closed this Apr 10, 2025
@Aidyn-A Aidyn-A reopened this Apr 30, 2025
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Apr 30, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased eigen_for_sparse_addmm onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout eigen_for_sparse_addmm && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the eigen_for_sparse_addmm branch from cec2a79 to e92166b Compare April 30, 2025 16:14
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented May 5, 2025

@cpuhrsch re-opening this PR. Is there anything I can do to resolve the internal failures?

@github-actions github-actions bot closed this Jun 4, 2025
pytorchmergebot pushed a commit that referenced this pull request Aug 20, 2025
…155357)

This pull request adds the following ops for sparse matrices using Eigen library:
```python
    add(a_csr, b_csr)
    add(a_csc, b_csc)

    addmm(c_csr, a_csr, b_csr)
    addmm(c_csr, a_csr, b_csc)
    addmm(c_csr, a_csc, b_csc)
    addmm(c_csr, a_csc, b_csr)

    addmm(c_csc, a_csr, b_csr)
    addmm(c_csc, a_csr, b_csc)
    addmm(c_csc, a_csc, b_csc)
    addmm(c_csc, a_csc, b_csr)
```

Currently, the operations for sparse matrices on CPU are available through MKL only. The non-existence of MKL on `aarch64` causes the unavailability of these ops on any machines with ARM based CPUs, including Apple Silicon, AWS Graviton and NVIDIA Grace. This PR addresses this issue by using Eigen as a backend for the above ops.

This is a re-factored version of my previous PR #101814. The main difference with the old one, this does not enable Eigen by default.
Pull Request resolved: #155357
Approved by: https://github.com/pearu, https://github.com/eqy
pytorchmergebot pushed a commit that referenced this pull request Aug 23, 2025
…155357)

This pull request adds the following ops for sparse matrices using Eigen library:
```python
    add(a_csr, b_csr)
    add(a_csc, b_csc)

    addmm(c_csr, a_csr, b_csr)
    addmm(c_csr, a_csr, b_csc)
    addmm(c_csr, a_csc, b_csc)
    addmm(c_csr, a_csc, b_csr)

    addmm(c_csc, a_csr, b_csr)
    addmm(c_csc, a_csr, b_csc)
    addmm(c_csc, a_csc, b_csc)
    addmm(c_csc, a_csc, b_csr)
```

Currently, the operations for sparse matrices on CPU are available through MKL only. The non-existence of MKL on `aarch64` causes the unavailability of these ops on any machines with ARM based CPUs, including Apple Silicon, AWS Graviton and NVIDIA Grace. This PR addresses this issue by using Eigen as a backend for the above ops.

This is a re-factored version of my previous PR #101814. The main difference with the old one, this does not enable Eigen by default.
Pull Request resolved: #155357
Approved by: https://github.com/pearu, https://github.com/eqy

Co-authored-by: Eli Uriegas <eliuriegas@meta.com>
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
…ytorch#155357)

This pull request adds the following ops for sparse matrices using Eigen library:
```python
    add(a_csr, b_csr)
    add(a_csc, b_csc)

    addmm(c_csr, a_csr, b_csr)
    addmm(c_csr, a_csr, b_csc)
    addmm(c_csr, a_csc, b_csc)
    addmm(c_csr, a_csc, b_csr)

    addmm(c_csc, a_csr, b_csr)
    addmm(c_csc, a_csr, b_csc)
    addmm(c_csc, a_csc, b_csc)
    addmm(c_csc, a_csc, b_csr)
```

Currently, the operations for sparse matrices on CPU are available through MKL only. The non-existence of MKL on `aarch64` causes the unavailability of these ops on any machines with ARM based CPUs, including Apple Silicon, AWS Graviton and NVIDIA Grace. This PR addresses this issue by using Eigen as a backend for the above ops.

This is a re-factored version of my previous PR pytorch#101814. The main difference with the old one, this does not enable Eigen by default.
Pull Request resolved: pytorch#155357
Approved by: https://github.com/pearu, https://github.com/eqy
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
…ytorch#155357)

This pull request adds the following ops for sparse matrices using Eigen library:
```python
    add(a_csr, b_csr)
    add(a_csc, b_csc)

    addmm(c_csr, a_csr, b_csr)
    addmm(c_csr, a_csr, b_csc)
    addmm(c_csr, a_csc, b_csc)
    addmm(c_csr, a_csc, b_csr)

    addmm(c_csc, a_csr, b_csr)
    addmm(c_csc, a_csr, b_csc)
    addmm(c_csc, a_csc, b_csc)
    addmm(c_csc, a_csc, b_csr)
```

Currently, the operations for sparse matrices on CPU are available through MKL only. The non-existence of MKL on `aarch64` causes the unavailability of these ops on any machines with ARM based CPUs, including Apple Silicon, AWS Graviton and NVIDIA Grace. This PR addresses this issue by using Eigen as a backend for the above ops.

This is a re-factored version of my previous PR pytorch#101814. The main difference with the old one, this does not enable Eigen by default.
Pull Request resolved: pytorch#155357
Approved by: https://github.com/pearu, https://github.com/eqy

Co-authored-by: Eli Uriegas <eliuriegas@meta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request open source release notes: sparse release notes category Stale triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants