Replace Small Array Allocation with StaticArrays#5
Conversation
| operation(::AbstractArray) = SymbolicUtils.Code.create_array | ||
| arguments(x::AbstractArray) = [SymbolicUtils.Code.create_array, size(x), x] |
There was a problem hiding this comment.
Additionally, the arguments implementation is incorrect even if this wasn't type piracy. Why do the arguments also contain create_array if the operation is supposedly create_array? create_array takes the following arguments:
- the array type
- the eltype (or
nothing) Val(ndims)Val(size)- the elements of the array (splatted)
Assuming we always use the "full" signature. The shorter signature doesn't have ndims, but it's valid to just ignore that.
There was a problem hiding this comment.
Yeah... the thing is that arguments behaves very weirdly and seems to splat the fields of a struct at various times. Not sure if that is expected. This is me trying to work around that behaviour and forward the case we hit an array to the same path as create_array which does actually work.
There was a problem hiding this comment.
well this is now faithful to what arguments shuold have returned now, but the implementation seems to be buried deep within TermInterface which seems like it wants to do a generic implementation. Can this definition just be slotted in
| Dispatches on operation type for type-safe extraction. | ||
| """ | ||
| get_dims_arg(::typeof(SymbolicUtils.array_literal), args) = args[1] | ||
| get_dims_arg(::typeof(SymbolicUtils.Code.create_array), args) = args[2] |
There was a problem hiding this comment.
Would also need to change to args[4] with the correct arguments definition.
There was a problem hiding this comment.
yeah, this is done now
| if issym(var) | ||
| for p in expr.pairs | ||
| if lhs(p) === var | ||
| return iscall(p) ? nothing : rhs(p) |
There was a problem hiding this comment.
Isn't p either an Assignment or DestructuredArgs? Would these ever return true from iscall?
There was a problem hiding this comment.
Also, isn't this function essentially identical to resolve_dims_argument?
| end | ||
|
|
||
| function infer_hvncat_dimensions(::typeof(SymbolicUtils.Code.create_array), args, expr::Code.Let) | ||
| args[2] |
There was a problem hiding this comment.
The dims are the 4th argument to create_array
| Limit to 4×4 (16 elements) for optimal performance. | ||
| """ | ||
| function is_small_hvncat(rows::Int, cols::Int) | ||
| return rows <= 4 && cols <= 4 && rows * cols <= 16 |
There was a problem hiding this comment.
Why not just rows * cols <= 16?
There was a problem hiding this comment.
This is done as well
| dims === nothing && continue | ||
|
|
||
| # Check if small enough for StaticArrays | ||
| # is_small_hvncat(dims[1], dims[2]) || continue |
There was a problem hiding this comment.
Why is this validation commented?
There was a problem hiding this comment.
There are cases where some of the dims are calculated on the fly or might look like (3,3) but is acttually not the literal tuple but a different type, which fails the check. This is known, and something I am looking at fixing.
There was a problem hiding this comment.
This is enabled now
| T = vartype(lhs_var) | ||
|
|
||
| # Fallback: use literal dims values for hcat/vcat cases | ||
| if length(dims) == 2 && dims[2] == 1 |
There was a problem hiding this comment.
A (3, 1) matrix is not the same as a (3,) vector. They're different types and dispatch differently.
There was a problem hiding this comment.
We can simplify this to retain types. This currently comes from us adding a trailing singular dimension in the detection check, and controlling for that by discarding it in favour of a Vector.
There was a problem hiding this comment.
This shuold be improved now
2 participants
No description provided.