-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
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
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
) |
Would it help to do this transformation in |
|
Ok, I have now changed this to do the transformation during linearization instead of expanding to |
What this PR does is expand for example
to
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 inexpand-if
?fixes #39611