Conversation
|
TODOs here:
Since I'll be out for 3 weeks everyone should feel free to just push to this. |
e174ddb to
6394bc8
Compare
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Overall looks great, I think the main thing I am wondering about is about the interface decision around wether we implement this as a set of BatchedAlg versions, or rather as a set of batched_f(...) functions. I definitely like using dispatch for switching between the strided and non-strided inputs, but I am wondering if there might be a benefit to really having a batched_svd_compact etc function.
This is also partially since that allows us to have a CPU version for this as well, so we can just offload all of this from TensorKit to here, (and possibly play with multithreading?).
|
I mostly agree with Lukas here, I think I would prefer
|
This is already implemented in a separate branch I have over at TensorKit, I can move it over here of course! |
e20efbd to
cb6a8aa
Compare
|
Latest commit gets rid of the
|
|
One remaining thing to do here is restore the specific path that targets the AMDGPU |
57afe25 to
f1b1b3a
Compare
|
Moving the bisection (batched and non) logic out of here to make this easier to understand |
863db9f to
1bcbe28
Compare
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Am I correct that we are still assuming all arrays to be of the same size for the batched svd? I know this is more in line with what vendors provide, but I don't immediately see how that will be useful for TensorKit, in general we have completely different sizes for the blocks?
|
We now do support ragged block sizes but the vendors only accept uniform batches, so the padding support is now present here. For lots of small blocks I think this still gives a performance benefit because the latency of launching a lot of small, individual SVDs is extremely bad. |
|
Regarding the equal versus unequal sizes. Do we know of any library providing support for any operation (matrix multiplication or some decomposition) in batched mode with unequal sizes? If not, we can make it a MatrixAlgebraKit policy to also only provide I guess many different strategies will be worth investigating. I can imagine something where say, 90%, of the blocks is promoted to their largest common size by padding, but then the 10% largest blocks are still just ran serially in order to avoid having to promote everything to unreasonable size. |
I don't know of any. I guess it's a bit frustrating to have been told to move the ragged handling in here only to now have to move it back out again, but if that's what everyone prefers we can do it. |
|
Oh sorry, I had overlooked this and probably didn't fully associate the word ragged with this. In that case, there is no need to remove this. Even if we want to follow more advanced strategies where only part of the blocks are treated via batched_svd, we can still use (ragged) batched_svd here for those blocks. |
|
That sounds good to me. I don't know if "ragged" is the generally accepted term (is there even one?) but we can also revisit this in the future, of course. |
|
I do have to say that I don't think I fully agree that just because there are no libraries that provide ragged batched implementations that we shouldn't try having an interface for this anyways, because no matter what we can't really get around the problem that we will need this. In any case we will have to put the implementation either here or in TensorKit, and my feeling is that it makes sense to put such a "kernel" in MatrixAlgebraKit since this is really just matrix algebra. This being said, since there are definitely different things that can be done in a truly uniform batched case it could make sense to have these as separate functions, and in that case this PR can be focusing on the uniform case? If I recall correctly, MKL has something like this for matmul as well, where there is batched and grouped batched versions to have uniform sizes within a single group and varying between groups |
Co-authored-by: Lukas Devos <ldevos98@gmail.com> Co-authored-by: Jutho <Jutho@users.noreply.github.com>
Co-authored-by: Jutho <Jutho@users.noreply.github.com>
c2a57aa to
382f50e
Compare
| rest = Int[] | ||
| end | ||
| return batches, rest | ||
| end |
There was a problem hiding this comment.
I don't fully understand the logic with pad == true here. I am fine without padding, batches contains lists of indices of matrices with same size, and their corresponding size. rest contains all other matrices (sizes that don't occur frequently enough, or sizes that are too large).
Padding will try to put everything in rest into a single batch, by padding them to the same (maximal) size. If that fails (because e.g. the maximal size is too big), it just gives up.
I would rather think that, with padding, we want to create a single batch of those matrices that are smaller than batch_size_limit, and then only keep those with a larger size in rest.
There was a problem hiding this comment.
This, of course, goes into the whole discussion of batching strategies.
There was a problem hiding this comment.
Yeah, I'm not sure this is optimal but my thinking was to kick off the first batch ASAP while we pad out and construct the rest. But maybe we can delay the full investigation of the best strategy for a later PR?
| # these MUST be "full" sized | ||
| Ṽ = similar(Vᴴ, (n, n, batch_size)) | ||
| Ũ = similar(U, (m, m, batch_size)) |
There was a problem hiding this comment.
Another undocumented CUSOLVER "feature" ?
There was a problem hiding this comment.
🫠 It's a journey of discovery
This branch has not been deployed
3 participants
Basically what it says on the tin. For CTMRG and other algorithms, we're getting absolutely slaughtered on GPU performance for TensorMaps with sectors because we have to spin up huge numbers of very small SVDs. I'm wrapping the batched SVDs each library provides to try to address this. Extremely open to comments but I wanted to get this rolling so I can unblock others.