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

de-duplicate union type splitting inference for duplicate loads of immutable fields #39888

Open
jakobnissen opened this issue Mar 2, 2021 · 5 comments
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) kind:feature Indicates new feature / enhancement requests

Comments

@jakobnissen
Copy link
Contributor

jakobnissen commented Mar 2, 2021

Edit 2023-03-17: Reduced example

julia> struct Foo
           x::Union{Int, Nothing}
       end

julia> function foo(x::Foo)
           x.x === nothing ? x.x : x.x + 1
       end;

julia> @code_native foo(Foo(1))

Here is a small working example:

using ErrorTypes # version 0.3

@inline f(x::Int)::Option{Int} = iszero(x) ? none : some(x + 1)
@inline g(x)::Option{Int} = some(@? f(x))
@inline h(x) = unwrap_or(g(x), 1)

Now check g(1):

  • @code_warntype shows some type instability here, e.g. the final some appears to be able to be an Option{Nothing}, which would throw an error on the typeassert
  • @code_native shows no error-throwing code, so it conflicts with @code_warntype.

Another issue which may or may not be related: Check @code_native h(1). It compiles to a single leaq instruction as expected. However, when putting it in a loop:

function foo(x)
    y = 0
    for i in x
        y += h(i)
    end
    y
end

it does not vectorize, even though this should be very easy to vectorize.

@jakobnissen
Copy link
Contributor Author

Okay, I have finally reduced the problem:

using ErrorTypes
f(x) = iszero(x) ? none(Int) : some(x)

function good(x)
    y = f(x)
    data = y.data
    data isa Ok ? some(data._1) : convert(Option{Int}, Err(data._1))
end

function bad(x)
    y = f(x)
    y.data isa Ok ? some(y.data._1) : convert(Option{Int}, Err(y.data._1))
end

Two issues:

  • In bad, despite checking that y.data isa Ok, this information is not given to the compiler.
  • Even though the compiler has no inferred what the type of y.data._1 in the latter branch is (and that an error is impossible), @code_native does not show any code related to throwing an error.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 2, 2021

Yes, we don't forward information from duplicate loads. This is one of several reasons we typically try to recommend using the native sum type Union{Some{T}, Exception} instead of building with a typed-error struct such an ExceptionOr{T}

@vtjnash vtjnash changed the title Code introspection / optimization error de-duplicate union type splitting inference for duplicate loads of immutable fields Mar 2, 2021
@vtjnash vtjnash added kind:feature Indicates new feature / enhancement requests compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Mar 2, 2021
@jakobnissen
Copy link
Contributor Author

Just to be sure - does this also explain the fact that @code_native and @code_warntype disagree about what is actually inferred at a given point in time?

@JeffBezanson
Copy link
Sponsor Member

@code_warntype runs only inference and not optimization passes by default, but @code_typed runs both. The thinking is that the output of code_warntype is easier to read if it's closer to the original code (inlining etc. can really do a number on it).

jakobnissen added a commit to jakobnissen/julia that referenced this issue Aug 31, 2022
Since Julia unfortuantely still cannot propagate information from immutable
fields loaded twice (see JuliaLang#39888, JuliaLang#41199), these kind of changes are necessary
to achieve type stability.
KristofferC pushed a commit that referenced this issue Aug 31, 2022
Since Julia unfortuantely still cannot propagate information from immutable
fields loaded twice (see #39888, #41199), these kind of changes are necessary
to achieve type stability.
@jakobnissen
Copy link
Contributor Author

This will be closed by #47574, if it gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) kind:feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

3 participants