-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[release-1.12] Workaround poor inlining behavior for if @generated
functions
#58996
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
[release-1.12] Workaround poor inlining behavior for if @generated
functions
#58996
Conversation
Restores something very close to the previous inlining behavior, without reverting the change to statically-encode `MethodError`s in the IR. This built-in should not actually have a high cost, but it coincidentally encourages our compiler not to inline wide calls to generators that it will never be able to refine to the IR / type-information it would have gotten by exploring the more narrow call to begin with.
This sounded more like a bug with compute_sparams cost? We know that function is much more expensive than dynamic dispatch, so it should never inline unless the user explicitly overrides the heuristics |
It probably is, but I wasn't sure if there isn't also arguably a separate bug around whether we should inline In any case, this PR is just a "spiritual revert" to get back to the old behavior before we get the proper fix(es) in |
I think this revert is fine. Having said that now inlining won't work in cases like the one below for example, so I still think it would be better to fix the root problem if possible. julia> func(::Int) = :int;
julia> func(::String) = :string;
julia> callfunc(x) = func(x);
julia> callcallfunc(x) = callfunc(x);
julia> code_typed(callcallfunc, (Any,))
1-element Vector{Any}:
CodeInfo(
1 ─ %1 = dynamic Main.callfunc(x)::Symbol
└── return %1
) => Symbol |
I made #59013 based on #58996 (comment). Does that seem like a better approach than this PR? |
I'm going to merge this (just so that we have an easy-to-justify fix on 1.12) but I like the direction you're taking in #59013 @serenity4 |
3960ef4
into
JuliaLang:backports-release-1.12
…functions (JuliaLang#58996) Restores something very close to the previous inlining behavior, without reverting JuliaLang#54972 This is a hack to workaround JuliaLang#58915 (comment) for 1.12, but we should leave an issue open so that we can put in a proper fix to inlining for the next major version. Also improves JuliaLang#58915 (comment), which was a dynamic `call` on 1.11 and a poorly-chosen `invoke` of the generator fallback on 1.12. This is now an inlined version of the non-fallback path for the generator: ```julia julia> nt = (next = zero(UInt32), prev = zero(UInt32)) (next = 0x00000000, prev = 0x00000000) julia> f(nt) = @inline Base.setindex(nt, 2, :next) f (generic function with 1 method) julia> @code_warntype optimize=true f(nt) MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32}) from f(nt) @ Main REPL[2]:1 Arguments #self#::Core.Const(Main.f) nt::@NamedTuple{next::UInt32, prev::UInt32} Body::@NamedTuple{next::Int64, prev::UInt32} 1 ─ %1 = builtin Base.getfield(nt, :prev)::UInt32 │ %2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32} └── return %2 ```
…functions (JuliaLang#58996) Restores something very close to the previous inlining behavior, without reverting JuliaLang#54972 This is a hack to workaround JuliaLang#58915 (comment) for 1.12, but we should leave an issue open so that we can put in a proper fix to inlining for the next major version. Also improves JuliaLang#58915 (comment), which was a dynamic `call` on 1.11 and a poorly-chosen `invoke` of the generator fallback on 1.12. This is now an inlined version of the non-fallback path for the generator: ```julia julia> nt = (next = zero(UInt32), prev = zero(UInt32)) (next = 0x00000000, prev = 0x00000000) julia> f(nt) = @inline Base.setindex(nt, 2, :next) f (generic function with 1 method) julia> @code_warntype optimize=true f(nt) MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32}) from f(nt) @ Main REPL[2]:1 Arguments #self#::Core.Const(Main.f) nt::@NamedTuple{next::UInt32, prev::UInt32} Body::@NamedTuple{next::Int64, prev::UInt32} 1 ─ %1 = builtin Base.getfield(nt, :prev)::UInt32 │ %2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32} └── return %2 ```
…functions (#58996) Restores something very close to the previous inlining behavior, without reverting #54972 This is a hack to workaround #58915 (comment) for 1.12, but we should leave an issue open so that we can put in a proper fix to inlining for the next major version. Also improves #58915 (comment), which was a dynamic `call` on 1.11 and a poorly-chosen `invoke` of the generator fallback on 1.12. This is now an inlined version of the non-fallback path for the generator: ```julia julia> nt = (next = zero(UInt32), prev = zero(UInt32)) (next = 0x00000000, prev = 0x00000000) julia> f(nt) = @inline Base.setindex(nt, 2, :next) f (generic function with 1 method) julia> @code_warntype optimize=true f(nt) MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32}) from f(nt) @ Main REPL[2]:1 Arguments #self#::Core.Const(Main.f) nt::@NamedTuple{next::UInt32, prev::UInt32} Body::@NamedTuple{next::Int64, prev::UInt32} 1 ─ %1 = builtin Base.getfield(nt, :prev)::UInt32 │ %2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32} └── return %2 ```
Restores something very close to the previous inlining behavior, without reverting #54972
This is a hack to workaround #58915 (comment) for 1.12, but we should leave an issue open so that we can put in a proper fix to inlining for the next major version.
Also improves #58915 (comment), which was a dynamic
call
on 1.11 and a poorly-choseninvoke
of the generator fallback on 1.12. This is now an inlined version of the non-fallback path for the generator: