Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport fix for what may be a memory-alignment issue? #34231

Closed
timholy opened this issue Jan 1, 2020 · 8 comments
Closed

Backport fix for what may be a memory-alignment issue? #34231

timholy opened this issue Jan 1, 2020 · 8 comments

Comments

@timholy
Copy link
Member

timholy commented Jan 1, 2020

Does anyone happen to know which PR likely fixed this, and whether it's backportable? I can do a bisect if it would be helpful.

julia> using Images

julia> input = Matrix{RGB{N0f8}}(undef, 5, 5);

julia> fill!(input, RGB(0, 0, 0));

julia> useother = trues(size(input));

julia> function transfermask(input, useother, othervalue)
           combined = similar(input)
           for idx in eachindex(combined)
               combined[idx] = ifelse(
                   useother[idx],
                   RGB{Float64}(othervalue),
                   input[idx]
               )
           end
           return combined
       end
transfermask (generic function with 1 method)

Buggy result (at least julia 1.0.5 and julia 1.2.0, maybe others):

julia> transfermask(input, useother, RGB{N0f8}(1.0,1.0,1.0))
5×5 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)
 RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)  RGB{N0f8}(1.0,0.0,0.0)

Correct result (at least 1.3.1 and master, maybe others):

julia> transfermask(input, useother, RGB{N0f8}(1.0,1.0,1.0))
5×5 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)

xref JuliaImages/Images.jl#814

@vtjnash
Copy link
Member

vtjnash commented Jan 2, 2020

#33706 perhaps?

@kimikage
Copy link
Contributor

kimikage commented Jan 3, 2020

I'm not familiar with LLVM, so I'm not sure #33706 is exactly what we are looking for.
FWIW, the following tests are all passed even on v1.0.5:

# issue 33590
function f33590(b, x)
y = b ? nothing : (x[1] + 1,)
return something(ifelse(b, x, y))
end
@test f33590(true, (3,)) == (3,)
@test f33590(false, (3,)) == (4,)

Also, FWIW, the following is a minimal code without Images.jl.

julia> Float64[ifelse(i > 1, 2//3, -1.0) for i=1:2] # v1.3.1 (or v1.3.0)
2-element Array{Float64,1}:
 -1.0
  0.6666666666666666
julia> Float64[ifelse(i > 1, 2//3, -1.0) for i=1:2] # v1.0.5 (the result depends on the uninitialized stack)
2-element Array{Float64,1}:
 -1.0
  0.9976410051612077

@galenlynch
Copy link
Contributor

Array assignment isn't necessary to get strange behavior:

julia> for i = 1:2 # julia1.2
       display(ifelse(i > 1, 1//3, -1.0))
       end
-1.0
1//140412727051408

@kimikage
Copy link
Contributor

kimikage commented Jan 3, 2020

You are right. I just thought it would be better not to depend on Images.jl, if someone runs the bisect. There are no further implications. 😄

@galenlynch
Copy link
Contributor

galenlynch commented Jan 3, 2020

The buggy display behavior seems to happen in Julia 1.0.5 as well

@timholy
Copy link
Member Author

timholy commented Jan 3, 2020

I tried applying #33706 to release-1.2 but it doesn't apply cleanly.

@kimikage
Copy link
Contributor

kimikage commented Jan 4, 2020

It might be a good idea to resolve it as a part of #34011.
I'm sorry I cannot be of any help due to insufficient resources.

@brenhinkeller
Copy link
Contributor

Looks like it has been long enough that a backport is presumably no longer relevant. Can confirm we're still getting the correct behaviour nowadays on 1.8.3:

julia> transfermask(input, useother, RGB{N0f8}(1.0,1.0,1.0))
5×5 Array{RGB{N0f8},2} with eltype RGB{N0f8}:
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)
 RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(1.0,1.0,1.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants