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

improve constraint propagation with multiple || #39618

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

simeonschaub
Copy link
Member

@simeonschaub simeonschaub commented Feb 11, 2021

What this PR does is expand for example

if x === nothing || x < 0
    return 0
end

to

if x === nothing
    @goto l
else
    if x < 0
        @label l
        return 0
    end
end

in lowering, which IIUC should correspond to the first option @vtjnash proposed in https://github.com/JuliaLang/julia/pull/39549/files#r573995564. Are there any potential problems with emitting gotos here? Or is there a nicer way to fix this than in expand-if?
fixes #39611

What this PR does is expand for example
```julia
if x === nothing || x < 0
    return 0
end
```
to
```julia
if x === nothing
    @goto l
else
    if x < 0
        @Label l
        0
    end
end
```
in lowering, which IIUC should correspond to the first option @vtjnash proposed in https://github.com/JuliaLang/julia/pull/39549/files#r573995564. Are there any potential problems with emitting `goto`s here? Or is there a nicer way to fix this than in `expand-if`?
fixes #39611
@simeonschaub simeonschaub added compiler:lowering Syntax lowering (compiler front end, 2nd stage) compiler:inference Type inference labels Feb 11, 2021
@simeonschaub
Copy link
Member Author

simeonschaub commented Feb 11, 2021

Hmm, seems like our analysis for closures really doesn't like this change:

julia> function _hypot(x...)
           maxabs = maximum(x)
           if iszero(maxabs) || isinf(maxabs)
               return maxabs
           else
               return y -> maxabs
           end
       end
_hypot (generic function with 1 method)

julia> @code_lowered _hypot(0)
CodeInfo(
1 ──       Core.NewvarNode(:(#7))
│          maxabs@_4 = Core.Box()
│    %3  = Main.maximum(x)
│          Core.setfield!(maxabs@_4, :contents, %3)
│    %5  = Core.isdefined(maxabs@_4, :contents)
└───       goto #3 if not %5
2 ──       goto #4
3 ──       Core.NewvarNode(:(maxabs@_5))
└───       maxabs@_5
4 ┄─ %10 = Core.getfield(maxabs@_4, :contents)
│    %11 = Main.iszero(%10)
└───       goto #6 if not %11
5 ──       goto #10
6 ── %14 = Core.isdefined(maxabs@_4, :contents)
└───       goto #8 if not %14
7 ──       goto #9
8 ──       Core.NewvarNode(:(maxabs@_6))
└───       maxabs@_6
9 ┄─ %19 = Core.getfield(maxabs@_4, :contents)
│    %20 = Main.isinf(%19)
└───       goto #14 if not %20
10%22 = Core.isdefined(maxabs@_4, :contents)
└───       goto #12 if not %22
11 ─       goto #13
12 ─       Core.NewvarNode(:(maxabs@_7))
└───       maxabs@_7
13%27 = Core.getfield(maxabs@_4, :contents)
└───       return %27
14#7 = %new(Main.:(var"#7#8"), maxabs@_4)
└───       return #7
)

@simeonschaub
Copy link
Member Author

Would it help to do this transformation in compile-body instead?

@simeonschaub
Copy link
Member Author

Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:519
 Unexpected Pass
 Expression: Core.Compiler.return_type(fadd3, (typeof(aa),)) <: Array19745{<:Union{Float64, Missing}}
 Got correct result, please change to @test if no longer broken.
Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:956
 Unexpected Pass
 Expression: Core.Compiler.return_type(broadcast, Tuple{typeof(+), Vector{Int}, Vector{Union{Float64, Missing}}}) == Vector{<:Union{Float64, Missing}}
 Got correct result, please change to @test if no longer broken.
Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:959
  Expression: Core.Compiler.return_type(broadcast, Tuple{typeof(+), Vector{Int}, Vector{Union{Float64, Missing}}}) == AbstractVector{<:Union{Float64, Missing}}
   Evaluated: Vector{T} where T<:Union{Missing, Float64} == AbstractVector{var"#s10134"} where var"#s10134"<:Union{Missing, Float64}
Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:963
 Unexpected Pass
 Expression: Core.Compiler.return_type(+, Tuple{Vector{Int}, Vector{Union{Float64, Missing}}}) == Vector{<:Union{Float64, Missing}}
 Got correct result, please change to @test if no longer broken.
Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:966
  Expression: Core.Compiler.return_type(+, Tuple{Vector{Int}, Vector{Union{Float64, Missing}}}) == AbstractVector{<:Union{Float64, Missing}}
   Evaluated: Vector{T} where T<:Union{Missing, Float64} == AbstractVector{var"#s10136"} where var"#s10136"<:Union{Missing, Float64}
Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:969
 Unexpected Pass
 Expression: Core.Compiler.return_type(+, Tuple{Vector{Int}, Vector{Union{Float64, Missing}}}) == Vector{<:Union{Float64, Missing}}
 Got correct result, please change to @test if no longer broken.
Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:973
 Unexpected Pass
 Expression: Core.Compiler.return_type(broadcast, Tuple{typeof(tuple), Vector{Int}, Vector{Union{Float64, Missing}}}) == Vector{<:Tuple{Int, Any}}
 Got correct result, please change to @test if no longer broken.
Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/broadcast.jl:976
  Expression: Core.Compiler.return_type(broadcast, Tuple{typeof(tuple), Vector{Int}, Vector{Union{Float64, Missing}}}) == AbstractVector{<:Tuple{Int, Any}}
   Evaluated: Vector{T} where T<:Tuple{Int64, Any} == AbstractVector{var"#s10139"} where var"#s10139"<:Tuple{Int64, Any}

image

@simeonschaub
Copy link
Member Author

Ok, I have now changed this to do the transformation during linearization instead of expanding to symbolicgoto in expand-forms, which seems like the better place anyways. This seems to have fixed the issues with closures getting boxed unnecessarily. It also apparently improved inference in broadcasting, so it looks like this does give us some nice inference improvements in the wild as well.

@JeffBezanson JeffBezanson merged commit abd56cd into master Feb 12, 2021
@JeffBezanson JeffBezanson deleted the sds/constraints_oror branch February 12, 2021 21:25
@tkf tkf mentioned this pull request Mar 10, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
@KristofferC KristofferC added the backport 1.6 Change should be backported to release-1.6 label Jul 12, 2021
KristofferC pushed a commit that referenced this pull request Jul 12, 2021
@KristofferC KristofferC mentioned this pull request Jul 12, 2021
75 tasks
KristofferC pushed a commit that referenced this pull request Aug 31, 2021
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Sep 7, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type constraints from multiple || conditions don't propagate well
3 participants