Repository navigation
Conversation
|
Done! |
e6fa5a5 to
95b02b3
Compare
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| supports_add(C, A, β) || | ||
| return TO.tensoradd!(C, A, pA, conjA, α, β, StridedNative(), allocator) |
There was a problem hiding this comment.
I guess this is slightly out of line with our other backends, which simply error when called on unsupported input types, rather than falling back to the default. (Also not sure we want StridedNative instead of StridedBLAS?).
I definitely agree to have some fancier logic in selecting the default backends, but I'm not fully convinced it's necessary to fall back here
There was a problem hiding this comment.
Here I really think it is because hipTENSOR only supports an extremely minimal subset of element types. So if it's loaded you actually get less functionality unless we do this 😭
There was a problem hiding this comment.
I see, but I think my idea is that by default, we are not selecting backends, and then DefaultBackend() is used which ends up calling select_backend to figure out what backend to actually use. There, I fully agree that we should only select the HipTensorBackend whenever the element types are supported. However, this method is called after HipTensorBackend has already been selected, which should therefore never happen automatically for non-supported types, and in that case it might actually be desirable to get explicit errors?
There was a problem hiding this comment.
Yeah I see what you mean, to sort of flip things around here. It's quite frustrating they don't even support fp64 lol.
| # hipTENSOR's permutations and elementwise operations ignore the layout of the output, | ||
| # so go through a dense temporary instead | ||
| if !isdense(C) | ||
| C′ = StridedView(ROCArray{eltype(C)}(undef, size(C))) |
There was a problem hiding this comment.
Can we use tensoralloc_add here, with corresponding tensorfree at the end?
| iszero(β) || (C′ .= C) | ||
| TO.tensoradd!(C′, A, pA, conjA, α, β, backend, allocator) | ||
| C .= C′ |
There was a problem hiding this comment.
| iszero(β) || (C′ .= C) | |
| TO.tensoradd!(C′, A, pA, conjA, α, β, backend, allocator) | |
| C .= C′ | |
| TO.tensoradd!(C′, A, pA, conjA, α, Zero(), backend, allocator) | |
| if iszero(β) | |
| C .= C′ | |
| else | |
| C .= β .* C .+ C′ | |
| end |
I'm not sure if this actually makes sense, but it does reduce the maximal number of passes over the entire array to 2.
|
|
||
| # hipTENSOR's permutations and elementwise operations ignore the layout of the output, | ||
| # so go through a dense temporary instead | ||
| if !isdense(C) |
There was a problem hiding this comment.
This is a very unfortunate restriction, I think that is most of the TensorKit use cases...
| α::Number, β::Number, | ||
| backend::hipTENSORBackend, allocator | ||
| ) | ||
| supports_contract(C, A, B) || return TO.tensorcontract!( |
There was a problem hiding this comment.
Same comment about the fallback here
|
|
||
| # convert arguments | ||
| Ainds, Binds, Cinds = collect.(TO.contract_labels(pA, pB, pAB)) | ||
| A′ = materialize_conj(A, conjA) |
There was a problem hiding this comment.
Same comment about using the allocator to instantiate these temporaries and free them below
|
As a side note, it is starting to look like a proper test suite for contractions/permutations with various backends might become a reasonable addition, but obviously doesn't have to hold up this PR. |
hipTENSOR's list of supported operations is pretty thin, and also the library isn't supported on every AMD GPU