search

ROCm / rocSPARSE

Next generation SPARSE implementation for ROCm platform
https://rocm.docs.amd.com/projects/rocSPARSE/en/latest/
MIT License
116 stars 56 forks source link

rocsparse_spsm ignores dense matrix order #370

Closed jakub-homola closed 9 months ago

jakub-homola commented 10 months ago

What is the expected behavior

In the rocsparse_spsm function (link) (and maybe other functins as well), the memory order of the dense matrices (B and C) should be taken into account.

The minimum I expect: check if the orders of B and C are equal (if not then error). If the order is rowmajor, change the transB value to the opposite one and swap numrows and numcols.

What actually happens

The memory order of the dense matrices is completely ignored, and is assumed to be column-major, which is not explicitly mentioned in the documentation.

How to reproduce

I dont't have a simple example, but I think it is not necessary, since I can clearly see that the order is ignored in the code. Will provide if needed.

Environment

I am using rocsparse that comes with rocm-5.2.3, but I can see the same problem in the sources for rocm-6.0.0

jakub-homola commented 9 months ago

Oh, and another thing I noticed in rocsparse_spsm (should I rather create a new issue for this?).

In the docs, it says that trans_B is only applied to the right-hand-side matrix B, not the solution matrix C, which is consistent with cusparse. But in the code (e.g. here) it seems like you think that the operation is applied to both B and C, since you are just performing the in-place algorithm, which cannot work if B and C need different access pattern.

BTW I originally also thought that trans_B applies to both matrices, so my buggy code works only because you code is buggy too :D

YvanMokwinski commented 9 months ago

Oh, and another thing I noticed in rocsparse_spsm (should I rather create a new issue for this?).

In the docs, it says that trans_B is only applied to the right-hand-side matrix B, not the solution matrix C, which is consistent with cusparse. But in the code (e.g. here) it seems like you think that the operation is applied to both B and C, since you are just performing the in-place algorithm, which cannot work if B and C need different access pattern.

BTW I originally also thought that trans_B applies to both matrices, so my buggy code works only because you code is buggy too :D

Thank you, this is something that has already been reported from https://github.com/ROCm/rocSPARSE/issues/365 . We are working on it to fix the issue.

YvanMokwinski commented 9 months ago

Fixed SpSM refactoring: https://github.com/ROCm/rocSPARSE/commit/bb22be0101d8d41e4a719c0f39eec8d5ca6d5455

Thank you.