Support @inbounds in setindex!(::Array, ::AbstractArray, ::AbstractVector}

[nalimilan]

Previously this annotation wouldn't have any effect on Array, contrary to
what happens with the general AbstractArray fallback method.
Also always use @inbounds for X[count] since count depends on the length of I,
(which is checked), and mark setindex_shape_check() as a bounds check.

Fixes #20907.

[nalimilan]

Failures are #20906. We should probably run Nanosoldier if it passes the review, though.

[ararslan]

I've restarted CI now that #20906 has been merged.

[blakejohnson]

LGTM

[mbauman]

@nanosoldier runbenchmarks(ALL, vs = ":master")

[mbauman]

With regards to using @boundscheck blocks for setindex_shape_check, I think that's fine. We could eventually do the same for the general AbstractArray methods. I didn't worry too much about it since it's typically much cheaper than the real work.

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[blakejohnson]

That array comprehension slowdown is troubling. Can you reproduce that locally, @nalimilan?

[KristofferC]

It is a very noisy benchmark that warns every second time the benchmarks are run which is why I merged anyway. I can double check to make sure?

[simonster]

@_propagate_inbounds_meta includes @inline. I'm not sure it's a good idea to inline a function as big as setindex!(A::Array, X::AbstractArray, I::AbstractVector{Int}).

[blakejohnson]

Unfortunately, our @inbounds mechanism requires inlining in order to have any effect.

[simonster]

I am aware. It just seems weird to be inlining a 13 line function to make it possible to elide bounds checks. That can have serious consequences in terms of compilation time even for people not using @inbounds.

In this case, maybe we could do something like:

function setindex!(A::Array, X::AbstractArray, I::AbstractVector{Int})
    @_inline_meta
    @boundscheck return checked_setindex!(A, X, I)
    unchecked_setindex!(A, X, I)
end

The method splitting could be done automatically with a macro, although that may not be so easy this early in bootstrap.

[blakejohnson]

If we can make that work, I agree that it would be better.

[nalimilan]

Makes sense. Though that would be unsafe_setindex!.