`accumulate` algorithm selection for Metal implementation being overridden

[christiangnrd]

Currently, when calling accumulate() with a MtlArray, even though it dispatches to the Metal extension accumulate! but still attempts to use the decoupled lookback algorithm since the default non-extension accumulate has DecoupledLookback() as it's default arument, and it gets passed on to the metal accumulate! which happily tries to use DecoupledLookback.

AcceleratedKernels.jl/src/accumulate/accumulate.jl

Lines 229 to 261 in 111c89b

	function accumulate(
	op, v::AbstractArray, backend::Backend=get_backend(v);
	init,
	neutral=neutral_element(op, eltype(v)),
	dims::Union{Nothing, Int}=nothing,
	inclusive::Bool=true,
	# Algorithm choice
	alg::AccumulateAlgorithm=DecoupledLookback(),
	# GPU settings
	block_size::Int=256,
	temp::Union{Nothing, AbstractArray}=nothing,
	temp_flags::Union{Nothing, AbstractArray}=nothing,
	)
	dst_type = Base.promote_op(op, eltype(v), typeof(init))
	vcopy = similar(v, dst_type)
	copyto!(vcopy, v)
	accumulate!(
	op, vcopy, backend;
	init=init,
	neutral=neutral,
	dims=dims,
	inclusive=inclusive,
	alg=alg,
	block_size=block_size,
	temp=temp,
	temp_flags=temp_flags,
	)
	vcopy
	end

The quick solution would be to define accumulate in the Metal extension, but I think it would be better (assuming no performance impacts) to just not specify the default arguments in the higher level definitions, and leave them to the implementations.

This will reduce the amount of copy-pasting default arguments, and the docstrings will still let users know what the potential kwargs are.

[anicusan]

This is odd, the dispatch only happens on the positional arguments - in the main AK.accumulate, the default positional argument generates two functions:

AK.accumulate(op, v; kwargs...) = AK.accumulate(op, v, get_backend(v); kwargs...)
AK.accumulate(op, v, backend::ActualExplicitBackend; kwargs...)

The kwargs only get filled in after dispatching on the positional arguments - so if it goes through the Metal extensions, there the algorithm=ScanPrefixes(). We'd see errors in the test suite otherwise. Have you seen any case where the DecoupledLookback algorithm is used on Metal instead?

Either way, I like your idea of reducing the kwarg duplication (as long as we don't end up like Matplotlib with an endless pit of unspecified kwargs) - I'll look into your PRs now

[christiangnrd]

Have you seen any case where the DecoupledLookback algorithm is used on Metal instead?

I encountered it when I tried to replace the explicit algorithm selection in JuliaGPU/Metal.jl#590 with using the backend (since that's how it'll work in GPUArrays).

There is no AK.accumulate(op, v, backend::MetalBackend; kwargs...) since presumably you figured it would dispatch to the Metal extension accumulate!, which it does, but only after inheriting the default accumulate default alg kwarg. We could also define a specific accumulate for MetalBackend with ScanPrefixes as the default algorithm, but I think it would be better to avoid redefining a function that does the exact same thing but with a slightly different default.

[anicusan]

Aaah, you're right - I missed the accumulate instead of accumulate!. Your solution in #39 is great.