Broadcast shouldn't use inbounds when calling f #19188
Conversation
| @@ -337,3 +337,8 @@ end | |||
| @test broadcast(+, 1.0, (0, -2.0)) == (1.0,-1.0) | |||
| @test broadcast(+, 1.0, (0, -2.0), [1]) == [2.0, 0.0] | |||
| @test broadcast(*, ["Hello"], ", ", ["World"], "!") == ["Hello, World!"] | |||
|
|
|||
| # Ensure that broadcast doesn't use @inbounds when calling the function | |||
There was a problem hiding this comment.
The tests are run with --check-bounds=yes, no? What's the best way to make sure that this catches regressions?
mbauman
force-pushed
the
mb/broadcast-outbounds
branch
from
409650a to
940c111
Compare
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.
mbauman
force-pushed
the
mb/broadcast-outbounds
branch
from
940c111 to
7a1c358
Compare
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
There seem to be some relevant slowdowns. It seems like we should still be able to use |
|
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 Unfortunately, this uses |
|
But wouldn't adding @inline _broadcast_getindex(::Any, A::AbstractArray, I) = @inbounds A[I]be safe? Here, the indices |
|
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. |
Try to restore some performance by allowing inbounds propagation into _broadcast_getindex.
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels |
|
I don't think this is relevant on release-0.5 without #16986 |
* Broadcast shouldn't use inbounds when calling f 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. * Add inbounds elsewhere in broadcast Try to restore some performance by allowing inbounds propagation into _broadcast_getindex.
Previously,
broadcast(getindex, A, I...)would callgetindexwithin anat-inbounds context. This patch simply moves the function call so that it
will throw a BoundsError appropriately.
Fixes #19203.