Broadcast shouldn't use inbounds when calling f

[mbauman]

Previously, broadcast(getindex, A, I...) would call getindex within an
at-inbounds context. This patch simply moves the function call so that it
will throw a BoundsError appropriately.

Fixes #19203.

[mbauman]

The tests are run with --check-bounds=yes, no? What's the best way to make sure that this catches regressions?

[yuyichao]

test/boundscheck.jl

[mbauman]

Ah, thanks!

[stevengj]

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

[nanosoldier]

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

[stevengj]

There seem to be some relevant slowdowns. It seems like we should still be able to use @inbounds somewhere, maybe in _broadcast_getindex?

[mbauman]

This is a bug-fix. The loss in performance is entirely due to restoring bounds checks in places where they had been erroneously removed before. In order to re-gain that performance, we'd have to add @propagate_inbounds annotations to the entire call chain, and then annotate the broadcast calls — if they're safe — with @inbounds f.(...).

Unfortunately, this uses @noinline (I assume) to ensure type stability of the inner loop, which precludes the use of @propagate_inbounds.

[stevengj]

But wouldn't adding

@inline _broadcast_getindex(::Any, A::AbstractArray, I) = @inbounds A[I]

be safe? Here, the indices I are constructed by broadcast itself, and should always be inside the array bounds.

[mbauman]

Sure, but that's unrelated to this change. Edit: Oh, I see, you want to pair it with a perf enhancement. Makes sense and is simple. I'll do it tonight.

[mbauman]

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

[nanosoldier]

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

[tkelman]

I don't think this is relevant on release-0.5 without #16986