Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow StaticArray eltype in matmat{vec,mul} #1954

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

lcw
Copy link
Contributor

@lcw lcw commented Jun 13, 2023

Here we avoid promotion since it is not defined between scalar numbers
and static array types.

Fixes #1953

Co-authored-by: Mason McCallum masonamccallum@gmail.com

@maleadt
Copy link
Member

maleadt commented Jun 13, 2023

I'll let @dkarrasch chime in, who wrote this code.

Copy link
Contributor

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

Sorry for the breakage. Indeed, that promotion did not exist prior to my PR. I took it from LinearAlgebra. I believe it is required there because BLAS needs the constant scalars to be of the same type as eltype? I wonder what happens when you pass an integer 1 as alpha with float eltypes. Do gemv! and gemmEx! and gemm! accept that? Or is there some internal type promotion happening?

@lcw lcw force-pushed the lcw/fixmulforstaticarrays branch from 9e8f212 to eacdf32 Compare June 13, 2023 17:30
@lcw
Copy link
Contributor Author

lcw commented Jun 13, 2023

We just tried

julia> mul!(C, A, B, 3, 0)
3×1 CuArray{SVector{2, Float32}, 2, CUDA.Mem.DeviceBuffer}:
 [2.441199, 1.6931901]
 [2.1284773, 3.0013726]
 [4.8450527, 4.7269773]

julia> A = CuArray(rand(SVector{2, Float32}, 3, 3))
3×3 CuArray{SVector{2, Float32}, 2, CUDA.Mem.DeviceBuffer}:
 [0.327501, 0.991028]  [0.328234, 0.0684084]  [0.426921, 0.892227]
 [0.069393, 0.921621]  [0.74181, 0.508899]    [0.318923, 0.0873129]
 [0.152261, 0.157193]  [0.139696, 0.0202893]  [0.67665, 0.207444]

julia> B = CuArray(rand(Float32, 3, 1))
3×1 CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}:
 0.7732126
 0.28225046
 0.848472

julia> C = similar(A, 3, 1)
3×1 CuArray{SVector{2, Float32}, 2, CUDA.Mem.DeviceBuffer}:
 [0.0, 0.0]
 [0.0, 0.0]
 [0.0, 0.0]

julia> mul!(C, A, B, 3, 0)
3×1 CuArray{SVector{2, Float32}, 2, CUDA.Mem.DeviceBuffer}:
 [2.124308, 4.62784]
 [1.6008871, 2.790985]
 [2.193833, 0.909842]

julia> all(C .≈ 3*A*B)
true

and it seemed to work. I don't know where the promotion is happening.

@lcw lcw force-pushed the lcw/fixmulforstaticarrays branch from eacdf32 to 14a92d5 Compare June 14, 2023 17:01
Here we avoid promotion since it is not defined between scalar numbers
and static array types.

Co-authored-by: Mason McCallum <masonamccallum@gmail.com>
@lcw lcw force-pushed the lcw/fixmulforstaticarrays branch from 14a92d5 to 40d87c9 Compare June 22, 2023 17:13
@maleadt maleadt added the bugfix This gets something working again. label Jun 26, 2023
@maleadt maleadt merged commit 6dacd70 into JuliaGPU:master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This gets something working again.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recent mul! changes break multiplication with matrices that have StaticArray elements
3 participants