Is the compiler smart enough to remove this duplicate computation?

Is the compiler smart enough that it doesn’t compute diff(w; dims=3) in the following snippet twice?

w = rand(2, 3, 4)
@assert(
    all((diff(w; dims=3) .> 0) .| isnan.(diff(w; dims=3))),
    "Something's amiss."
)

If it isn’t, then I would rewrite as

w = rand(2, 3, 4)
f(x) = x > 0 | isnan(x)
@assert(
    all(f.(diff(w; dims=3))),
    "Something's amiss."
)

I don’t think it will, since there’s no guarantee that diff doesn’t modify w. Also, an array could be modified from some other thread. The output of @code_llvm and @code_native are much too long for me to be able to parse.

You should avoid this pattern in most cases:

all(f.(diff(w; dims=3)))

and instead do

all(f, diff(w; dims=3))

The first creates an unnecessary array allocation, and also calculates f for all inputs, even when it could bail out early.

I would just write:

function bar(w)
    @assert all(x->(x>0 | isnan(x)), diff(w; dims=3)) "Something's amiss."
end

BTW, you should be aware of when to use @assert (from the docstring):

 │ Warning
 │
 │  An assert might be disabled at various optimization levels. Assert should therefore only be used as a debugging tool and not used for authentication verification
 │  (e.g., verifying passwords), nor should side effects needed for the function to work correctly be used inside of asserts.

Unless you are aware of this already, it’s likely that you should throw an error instead.

8 Likes

Thank you! Stellar help, as always :slight_smile:

I would just like to add to the excellent previous answer that that solution is not only more efficient, but easier to read: you don’t have to visually parse twice the same blob (even when they look similar at first sight, developers will expect that they differ as otherwise they would have been factored out, so extra time is spent tracking the tiniest possible detail in which they could be different).

3 Likes

I would have thought no, too. But I like to experiment and have

using BenchmarkTools
using Random

test1(w) =
    all((diff(w; dims=3) .> 0) .| isnan.(diff(w; dims=3)))

test2(w) =
    all(x > 0 || isnan(x) for x in diff(w; dims=3))

test3(w) =
    all(x -> x > 0 || isnan(x), diff(w; dims=3))

Random.seed!(42)
w = rand(2, 3, 4)
r1 = test1(w) 
r2 = test2(w)
@assert r2 == r1
r3 = test3(w)
@assert r3 == r2

@btime test1(w) setup = (Random.seed!(42); w = rand(2, 3, 4))
@btime test2(w) setup = (Random.seed!(42); w = rand(2, 3, 4))
@btime test3(w) setup = (Random.seed!(42); w = rand(2, 3, 4))

yielding

  382.178 ns (4 allocations: 528 bytes) # all(f.(xs))
  126.239 ns (1 allocation: 208 bytes)   # all(for x in xs)
  125.670 ns (1 allocation: 208 bytes)   # all(f, xs)

And from this we conclude that it is in fact not smart enough as my original snippet takes more than twice as long and has four times as many allocations…?

Yes, currently. But I noticed one interesting development in the compiler which could change this behavior in the future AFAIU…

Edit: deleted due to testing mistake…